-
Notifications
You must be signed in to change notification settings - Fork 169
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 error signals from MoistAir Tests #4430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think that the test-program should be updated instead of removing near-zero signals, but approving in this case since the test is also redundant.
I agree. In cases lacking an assert, we should indeed make sure that near-zero variables are actually near zero. The problem is, how do we define near-zero 😄 As I understand, the problem is not (only) to update the test program, but primarily to add information to the reference trajectories about their order of magnitude (a.k.a. nominal attribute in Modelica). This information must be supplied explicitly, it cannot be inferred by just looking at the data values. Maybe we could extend the CSV file to provide one extra row (e.g. with NaN as time value) that contains this information? |
I have explained a solution that I think will handle it in practice which only require an update of the test program; see #4421 (comment) The key point is that just because we could have very small nominal values, that doesn't mean that we actually have them in practice. If we realize that a few signals need nominal signals after all we could then handle it.
That would create more problems for other uses of the CVS-files. |
I'm afraid the wrong missing information is being discussed here. What is missing is not the nominal magnitude of a variable, but its nominal dynamic range. Take ambient air pressure outside a building for example. The nominal magnitude is 1e5 Pa, but the nominal dynamic range is orders of magnitude smaller than that, and it is the latter which is relevant for setting comparison tolerances. If done wrong, quite large errors in air pressure might pass correctness checks just because the nominal magnitude happens to be a big number. |
Backporting this to maint 4.1.x by #4463 |
These signals are internally calculated to trigger asserts, but are themselves numerically unreliable, since they are by construction close to zero and only due to numerical error, so it makes no sense to compare them across tools or for regression testing.
Once accepted, this should be ported to maint/4.1.0