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 error signals from MoistAir Tests #4430

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

casella
Copy link
Contributor

@casella casella commented Jun 22, 2024

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

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.

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.

@casella
Copy link
Contributor Author

casella commented Jun 24, 2024

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?

@casella casella merged commit a1ebc8f into modelica:master Jun 24, 2024
2 checks passed
@HansOlsson
Copy link
Contributor

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 #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.

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
Copy link
Contributor

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.

@casella casella modified the milestone: MSL4.1.0 Jun 28, 2024
@beutlich beutlich added the L: Resources Issue addresses Modelica/Resources (excl. C-Sources) label Jul 19, 2024
@beutlich beutlich changed the title Removed error signals from MoistAir Tests Remove error signals from MoistAir Tests Jul 19, 2024
@Esther-Devakirubai
Copy link
Contributor

Backporting this to maint 4.1.x by #4463

casella added a commit that referenced this pull request Sep 10, 2024
…tests (#4430) (#4463)

Co-authored-by: Francesco Casella <francesco.casella@polimi.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Resources Issue addresses Modelica/Resources (excl. C-Sources)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants