ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

MSL 4.1.0 Regressions - Magnetic.FluxTubes

Open GallLeo opened this issue 1 year ago • 7 comments

The following models fail in result comparison. Tested revision: f9bddf86 (2024-02-16)

Changed models, need reference update after library officer check:

  • [x] Modelica.Magnetic.FluxTubes.Examples.MovingCoilActuator.ForceCurrentBehaviour - Reason: pmActuator.coil.L_stat is ill-defined at time = 0. @AHaumer will provide a PR to remove it from the comparison signals. Resolved by #4418 - Updated reference files? Yes pls. @GallLeo

  • [x] ModelicaTest.Magnetic.FluxTubes.Sensors - Reason: no more regression reported - Updated reference files? No.

  • [x] ModelicaTest.Magnetic.FluxTubes.Shapes.FixedShape - Reason: no more regression reported - Updated reference files? No.

  • [x] Release notes check: Are all classes mentioned which could lead to result changes in user models?

I have to dig into the first one, but the other two: @GallLeo are you sure? There's no diff at all: grafik


Useful Links

Current comparison report: https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/comparison_report_overview.html -> Reference result test -> Comparison

Comparison signal definitions: https://github.com/modelica/ModelicaStandardLibrary/tree/master/Modelica/Resources/Reference/Modelica https://github.com/modelica/ModelicaStandardLibrary/tree/master/ModelicaTest/Resources/Reference/ModelicaTest

Reference results: https://github.com/modelica/MAP-LIB_ReferenceResults

GallLeo avatar Feb 27 '24 08:02 GallLeo

In Modelica.Magnetic.FluxTubes.Examples.MovingCoilActuator.ForceCurrentBehaviour the variable pmActuator.coil.L_stat is just a bad choice. For numerical reasons it gets protected from a division by zero (when the current through the coil is near zero). This depends on interval (which is not defined!) and tolerance. Question 1: Should we exclude the mentioned variable from comparisonSignals.txt? Question 2: Should we define a meaningful interval in the experiment annotation for all 3 examples in Modelica.Magnetic.FluxTubes.Examples.MovingCoilActuator? @christiankral @GallLeo what's your opinion?

AHaumer avatar Mar 01 '24 20:03 AHaumer

In Modelica.Magnetic.FluxTubes.Examples.MovingCoilActuator.ForceCurrentBehaviour the variable pmActuator.coil.L_stat is just a bad choice. For numerical reasons it gets protected from a division by zero (when the current through the coil is near zero). This depends on interval (which is not defined!) and tolerance. Question 1: Should we exclude the mentioned variable from comparisonSignals.txt?

Yes indeed. This is a good proposal.

Question 2: Should we define a meaningful interval in the experiment annotation for all 3 examples in Modelica.Magnetic.FluxTubes.Examples.MovingCoilActuator? @christiankral @GallLeo what's your opinion?

This makes also sense. @AHaumer Please provide a PR

christiankral avatar Mar 10 '24 10:03 christiankral

Reopening since we need to backport #4418 to maint branch before closing.

maltelenz avatar Jun 24 '24 07:06 maltelenz

As I understand, we also need the new reference results to be generated and uploaded.

@maltelenz I would close the tickets once the action have been taken on master. I asked Modelon India to take care of the back-porting, they can go through all the 4.1.0 issues that have been closed in the last two months, collect the commits and make one big PR with all the back-ports.

casella avatar Jun 28 '24 21:06 casella

@maltelenz I would close the tickets once the action have been taken on master.

I disagree, then we would risk forgetting something for 4.1.0. If the issue is closed, what is supposed to remind us?

collect the commits and make one big PR with all the back-ports.

That would make it very hard to verify that the backporting is correct, since you cannot compare to the changes in the original PRs if everything is thrown together in a very big pile.

maltelenz avatar Jul 01 '24 06:07 maltelenz

We can keep it open until it gets backported. Note that we may have other tickets that have been closed already but not back-ported, so it's going to be messy anyway 😃

casella avatar Jul 01 '24 21:07 casella

No impact on user's models, no need to mention in the release notes. Can be closed once backported to maint/4.1.x

casella avatar Jul 31 '24 09:07 casella

As far as I can tell, nothing remains.

Closing.

maltelenz avatar Feb 11 '25 12:02 maltelenz