modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
452 stars 165 forks source link

Removed error signals from MoistAir Tests #4430

Closed casella closed 1 week ago

casella commented 1 week ago

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

casella commented 1 week ago

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?

HansOlsson commented 1 week ago

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.

I have explained a solution that I think will handle it in practice which only require an update of the test program; see https://github.com/modelica/ModelicaStandardLibrary/issues/4421#issuecomment-2178650426

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.

Maybe we could extend the CSV file to provide one extra row (e.g. with NaN as time value) that contains this information?

That would create more problems for other uses of the CVS-files.

henrikt-ma commented 1 week ago

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.