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
472 stars 168 forks source link

MSL 4.1.0 Reference results Modelica.Electrical.Analog.Examples.DifferenceAmplifier #4362

Open maltelenz opened 6 months ago

maltelenz commented 6 months ago

The model Modelica.Electrical.Analog.Examples.DifferenceAmplifier has changed stop time in the model:

https://github.com/modelica/ModelicaStandardLibrary/blob/677bec0bb2a9c1f9a4bfac9924322ed66a5199e5/Modelica/Electrical/Analog/Examples/DifferenceAmplifier.mo#L129

compared to the current reference results:

https://github.com/modelica/MAP-LIB_ReferenceResults/blob/2be1c7e08a39d658b1326607917a0a32749e76e1/Modelica/Electrical/Analog/Examples/DifferenceAmplifier/creation.txt#L45

which means we need new reference results.

@GallLeo I would also be interested in how it can be considered green in the report:

https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/Compare/Modelica/testrun_report.html

Maybe there is something wrong with how tests are run?

beutlich commented 6 months ago

I would also be interested in how it can be considered green in the report:

That's strange indeed. When trying to reproduce locally I get

The compare file contained 9 results. 9 results were tested. 5 results failed. Success rate is 44,4%.

and

2024-03-25Z17:31:03 [     Warning ] C1.v is invalid! 2737 errors have been found during validation.
2024-03-25Z17:31:03 [     Warning ] C3.v is invalid! 3711 errors have been found during validation.
2024-03-25Z17:31:03 [     Warning ] C5.v is invalid! 2485 errors have been found during validation.
2024-03-25Z17:31:03 [     Warning ] Transistor1.ct.v is invalid! 2918 errors have been found during validation.
2024-03-25Z17:31:04 [     Warning ] Transistor2.ct.v is invalid! 3711 errors have been found during validation.

compare_failed.log

MatthiasBSchaefer commented 5 months ago

We decided to run the MSL examples with the same tolerances, output-interval (or in general: the same simulation settings) as the reference file from MAP-LIB-Reference Repo. Thus, also the simulation setting "stop time" is set as defined in the reference file, as you can also see in https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/Testruns/Dymola/Modelica/_report/Modelica.Electrical.Analog.Examples.DifferenceAmplifier/testcase_report.html.

As soon as there is an updated reference file, we'll also compare with the new stop-time.

maltelenz commented 5 months ago

We decided to run the MSL examples with the same tolerances, output-interval (or in general: the same simulation settings) as the reference file from MAP-LIB-Reference Repo.

This seems deeply problematic to me, since that means it doesn't catch issues like these. If we don't catch issues like these, we do not know that we need to update the reference file in the first place.

MatthiasBSchaefer commented 5 months ago

Either we can take the simulation settings from the reference file or the simulation settings from the experiment annotation. Both is simply not possible.

maltelenz commented 5 months ago

Long term, I absolutely think we should use the experiment annotation from the model.

Short term (for the 4.1.0 release), I assumed we were using the the steps described in the pdf linked from here, specifically, something like this slide:

image

I guess I was mistaken in that assumption.

@casella may want to comment in his role as project leader.

Maybe for the 4.1.0 release, this particular problem is limited enough (I believe this is the only case I detected) that we should do the pragmatic thing and just update the reference file with a new one, after a library officer manually checks that the current result is OK?