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
475 stars 169 forks source link

Analysis of the MSL v3.2.3-rc.1+71590c7 regressions vs. v3.2.2+build.0-beta.3 #2783

Closed beutlich closed 5 years ago

beutlich commented 5 years ago

This is the public report of the 7 failed examples models for (untagged) MSL v3.2.3-rc.1+71590c7 vs. v3.2.2+build.0-beta.3 with my analysis, leaving 3 models open for further analysis.

Modelica\Blocks\Examples\NoiseExamples\ActuatorWithNoise\ActuatorWithNoise.csv

Modelica\Blocks\Examples\NoiseExamples\DrydenContinuousTurbulence\DrydenContinuousTurbulence.csv

The changes in Modelica.Blocks.Examples.NoiseExamples.ActuatorWithNoise and Modelica.Blocks.Examples.NoiseExamples.DrydenContinuousTurbulence are due to the fix in the calculation of the Modelica.Blocks.Interfaces.PartialNoise.localSeed based on function Modelica.Math.Random.Utilities.impureRandomInteger. In MSL 3.2.2 the localSeed was 5009188, in MSL 3.2.3 it is 5009189. This is due to https://github.com/modelica/ModelicaStandardLibrary/issues/2252. This is mentioned in the release notes. We still could change the example models to utilize the fixedLocalSeed of 5009188 for reproducable results.

Modelica\Electrical\Analog\Examples\AmplifierWithOpAmpDetailed\AmplifierWithOpAmpDetailed.csv

There was one change in #2634 that changed the value of pi for OpAmpDetailed but I cannot judge if this is relevant or not.

@AHaumer @christiankral @kristinmajetta @christophclauss Please add you analysis here. kristinmajetta: local introduced pi was changed into Modelica.constants.pi. I do not see why this should affect the results of OpAmpDetailed.

Modelica\Electrical\MultiPhase\Examples\Rectifier\Rectifier.csv

@AHaumer @christiankral Please add you analysis here.

Modelica\Fluid\Examples\Tanks\TanksWithOverflow\TanksWithOverflow.csv

@HansOlsson @casella Please add you analysis here.

Modelica\StateGraph\Examples\ExecutionPaths\ExecutionPaths.csv

This looks like a false positive. Whereas for the MSL v3.2.2 regression run the stop time event was stored as

...
14.994,0,1,0,0,0,0,0,0,0
14.997,0,1,0,0,0,0,0,0,0
15,0,1,0,0,0,0,0,0,0
15,0,1,0,0,0,0,0,0,0

it now is

...
14.994,0,1,0,0,0,0,0,0,0
14.997,0,1,0,0,0,0,0,0,0
15,0,1,0,0,0,0,0,0,0
15,1,0,0,0,0,0,0,0,0

@HansOlsson @GallLeo Please confirm.

Modelica\Utilities\Examples\ReadRealMatrixFromFile\ReadRealMatrixFromFile.csv

The behaviour of Modelica.Utilities.Examples.ReadRealMatrixFromFile is correct. The MSL 3.2.2 regression was based on v3.2.2-beta.2 and performed on 07.03.16, however there have been later changes that went into v3.2.2 due to https://github.com/modelica/ModelicaStandardLibrary/issues/1948. Thus, the MSL 3.2.2 reference result is wrong and no further action should be necessary.

AHaumer commented 5 years ago

Modelica\Electrical\Analog\Examples\AmplifierWithOpAmpDetailed\AmplifierWithOpAmpDetailed.csv

There was one change in #2634 that changed the value of pi for OpAmpDetailed but I cannot judge if this is relevant or not.

It seems that the exact value of pi has great influence. Simple workaround in Dymola2019: Set tolerance (from 2e-7) to 1e-6 and the result looks fine.

AHaumer commented 5 years ago

Modelica\Electrical\MultiPhase\Examples\Rectifier\Rectifier.csv

Besides adding power sensors (this is a good chance to compare and test different power sensors), the parameters of the model have been changed to more realistic values. Additionally, the initialization has been improved significantly starting nearly in steady state. Therefore the old reference results cannot match the new results. I recommend to generate new reference results with the new version.

HansOlsson commented 5 years ago

For ExecutionPaths I cannot reproduce the problem, but since transition{1,2}.enableFire does change at 15s if simulating longer I don't see a problem with having that change - or not having it.

For TanksWithOverflow it seems to be caused by changes of height_ab (and one additional height) due to https://github.com/modelica/ModelicaStandardLibrary/pull/2539 (at least the main variation). This was previously discussed in a mail-thread as well.

Thus from my side there are no remaining issues.