Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unreliable signals very close to zero #4420

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

casella
Copy link
Contributor

@casella casella commented Jun 18, 2024

These signals only depend on numerical approximations and CSV compare cannot evaluate them reliably because it lacks information about their magnitude (nominal attribute).

@casella
Copy link
Contributor Author

casella commented Jun 18, 2024

Once approved, this should be cherry-picked to maint/v4.1.0 as well.

@casella casella mentioned this pull request Jun 18, 2024
15 tasks
@casella casella enabled auto-merge (squash) June 18, 2024 22:57
@beutlich beutlich removed their request for review June 19, 2024 19:31
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would very much prefer if we could update the comparison program instead.

Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a specialist in MultiBody but for me it looks reasonable.

@casella
Copy link
Contributor Author

casella commented Jul 2, 2024

@HansOlsson the more I think of it the more I see you point. These signals are supposed to be near zero, and if we don't check they may (in principle) stray away.

One could argue that they very likely can't do that without some of the non-zero components changing significantly, but at the end of the day this issue can be really fixed by just adding some sensible absolute tolerance to the CVS-compare tool.

I'll keep this on hold for the time being.

@maltelenz maltelenz marked this pull request as draft July 3, 2024 06:39
auto-merge was automatically disabled July 3, 2024 06:39

Pull request was converted to draft

@maltelenz
Copy link
Contributor

I converted it to a draft PR to reflect the previous comment and avoid accidental merging.

@beutlich beutlich changed the title Removed unreliable signals very close to zero Remove unreliable signals very close to zero Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants