search

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 Regressions - Magnetic.FluxTubes #4336

Open GallLeo opened 7 months ago

GallLeo commented 7 months ago

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

Changed models, need reference update after library officer check:

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_

AHaumer commented 7 months ago

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?

christiankral commented 6 months ago

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

maltelenz commented 3 months ago

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

casella commented 3 months ago

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.

maltelenz commented 3 months ago

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

casella commented 3 months ago

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 commented 2 months ago

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