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

MSL 4.1.0 Reference results ModelicaTest.MultiBody.Forces.EngineGasForce #4359

Open
maltelenz opened this issue Mar 21, 2024 · 7 comments
Open
Labels
L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined ref-result Issue addresses the reference results
Milestone

Comments

@maltelenz
Copy link
Contributor

In #4081 variables were removed from the comparison signals. Hence, the reference results need to be updated.

@maltelenz maltelenz added this to the MSL4.1.0 milestone Mar 21, 2024
@maltelenz maltelenz added the ref-result Issue addresses the reference results label Mar 21, 2024
@beutlich beutlich changed the title MSL 4.1.0 Reference results Modelica.Mechanics.MultiBody.Examples.Loops.Utilities.GasForce2 MSL 4.1.0 Reference results ModelicaTest.MultiBody.Forces.EngineGasForce Mar 21, 2024
@beutlich
Copy link
Member

Duplicate of #4349 (does not matter if new or altered models) and already resolved in https://github.com/beutlich/MAP-LIB_ReferenceResults/blob/v4.1.0/ModelicaTest/MultiBody/Forces/EngineGasForce/EngineGasForce.csv where gasForce.dens no longer is listed in the simulation result CSV file. Can you live with Dymola 2014x reference results being available in https://github.com/beutlich/MAP-LIB_ReferenceResults?

@beutlich beutlich added the L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined label Mar 21, 2024
@maltelenz
Copy link
Contributor Author

Not a duplicate, the other issue is about new models.

I'm not satisfied with the fully regenerated baselines on your fork, maplib decided for higher ambitions this time, to only update references where we have a reason.

@beutlich
Copy link
Member

beutlich commented Mar 21, 2024

Not a duplicate, the other issue is about new models.

It is a duplicate in the sense that https://github.com/beutlich/MAP-LIB_ReferenceResults gives you both the new and the altered results. Anyway, take it or leave it. But it does not help to complain again and again.

only update references where we have a reason

I actually wonder how MAP-Lib can figure out a-priori which simulation results change given a singular change on a base component, say e.g. e5f0e7e. In my opinion you can only find out if you run a full regression. And in that case you get the altered results (as increment to the existing results). If a model is not affected, the CSV stays byte-identical and git can handle it.

@maltelenz
Copy link
Contributor Author

I don't see opening issues to keep track of what still needs to be done before a release as complaining.

@beutlich
Copy link
Member

beutlich commented Mar 21, 2024

Hm, I have to admit I am even more confused. Are you asking for the (as previously done) manual job of running the simulation of a tagged release in Dymola and providing them (as described in https://github.com/modelica/ModelicaStandardLibrary/wiki/Reference-results) or are you asking for the commit-triggered increments of simulation results provided automatically by a CI? For the former you have my answer, for the latter MAP-Lib is not yet there.

@maltelenz
Copy link
Contributor Author

maltelenz commented Mar 21, 2024

We already have regression runs that run Dymola, comparing new results against the expected results from 4.0.0. We also have a collection of reported issues where these runs generate different results.

The next step is that library officers make a decision for each case where the results differ. Either they decide that the new results are expected because of changes in the library, and the reference results can then be updated (for the specific model). Alternatively, the difference is unexpected, and something has to be fixed in the library before release.

Just updating all references misses the important step of verifying that the new results are correct. Even if one does verify that, updating references that were within tolerances for the regression run, risks that the results drift away over time.

Of course, doing this step for each commit or each PR during development in a CI fashion is preferable, but as you say, we aren't there yet. I believe this is the main thing @casella wants us to focus on once 4.1.0 is ready.

Edit: @GallLeo and colleagues have already volunteered to do the actual update of the individual references once library officers indicate this is the way forward for specific tests. I believe there is already a draft PR with that work ongoing.

@beutlich
Copy link
Member

Anyway, take it or leave it

Better leave it. After spending some more time on #4343 I only noticed now that some of the simulation result is no longer reproducable. Not sure what I did then (beside the too much manual patching of unusable MSL 4.1.0-beta.1). In order not to use it I removed that fork for now. Sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined ref-result Issue addresses the reference results
Projects
None yet
Development

No branches or pull requests

2 participants