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 - Blocks.Tables #4343

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 C-code/binaries, need reference update after library officer check:

The change is that the 2nd derivative of the output changes discontinuously exactly at the end of the simulation, whether we use left- or right-limit shouldn't matter. Optionally we could change the simulation stop time to make that clearer.

No signals to compare, define more comparison signals:

I propose to not do anything for these.

The issue is that the comparison signals for these surfaces are generated in ModelicaServices and are thus tool-specific (technically we should compare the animation results).

I propose to not do anything for these. The derivative change is just left- or right-limit.

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_

beutlich commented 7 months ago

Tested revision: f9bddf8 (2024-02-16)

f9bddf86b19027bf2223b3e5a29647a35550e5d5 is not part of v4.1.0-beta.1. Should it be da5c5de71f574b33a4f4631e09ba79682755f6d0?

beutlich commented 7 months ago

I am confused. I ran the result comparison of ModelicaTest.Tables of MSL4.1.0-beta.1 w.r.t. MSL v4.0.0 and get different comparison errors, see https://beutlich.github.io/MSLTEST410BETA1FAILED_Tables/

beutlich commented 7 months ago

We had two bug fixes for CombiTable2D, which fixed the interpolation and 2-nd derivatives in case of extrapolation by constant continuation:

https://github.com/modelica/ModelicaStandardLibrary/blob/da5c5de71f574b33a4f4631e09ba79682755f6d0/Modelica/Resources/C-Sources/ModelicaStandardTables.c#L47-L56

MatthiasBSchaefer commented 6 months ago

The regression tests of LTX cover the branch "master". We do NOT consider the branch maint/4.1.0 development seems to be performed in both branches, which is maximal confusing ...

beutlich commented 6 months ago

development seems to be performed in both branches, which is maximal confusing ...

Hm, it indeed doubles the workload, but it should not be confusing since main development happens on master and gets back-ported to maint branches if necessary or desired.

beutlich commented 6 months ago

@GallLeo I have problems in opening the comparison links, e.g. https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/Compare/ModelicaTest/20240316005556/ModelicaTest.Tables.CombiTable2Ds.Test26/CSVCompare/Test26_report.html

grafik

beutlich commented 6 months ago

The regression tests of LTX cover the branch "master".

Being called "MSL 4.1.0 Regressions" - they should no longer run from master, but from maint/4.1.0 branch.

beutlich commented 6 months ago

We had two bug fixes for CombiTable2D, which fixed the interpolation and 2-nd derivatives in case of extrapolation by constant continuation:

@HansOlsson Regarding ModelicaTest.Tables.CombiTable2Ds.Test26 the issue is in the very last step (at t = 1) of the second derivative no longer being -8 but 0 (as is the case for t > 1 s).

d2_t_new.y is invalid! 2 errors have been found during validation.

grafik

After rolling d0d158068b8f195c3c45703fc5b91eb3359f39e4 of #3893 back, I can confirm that the regression issue is gone. To me that change in the discontinuous second derivative looks strange.

beutlich commented 6 months ago

I am confused. I ran the result comparison of ModelicaTest.Tables of MSL4.1.0-beta.1 w.r.t. MSL v4.0.0 and get different comparison errors

Hm, not sure what I did then, but I finally can reproduce the regression report of ModelicaTest.Tables:

The compare directory contained 208 files. 196 files were tested. 4 files failed. Success rate is 98,0%.

beutlich commented 4 months ago

@HansOlsson Can you please define the expected value of ModelicaTest.Tables.CombiTable2Ds.Test26.d2_t_new.y at time=1? I believe it should be -8 (as is the case in MSL v4.0.0 and also sensible when loooking at d2_t_new.u) and not jump to zero.

HansOlsson commented 4 months ago

@HansOlsson Can you please define the expected value of ModelicaTest.Tables.CombiTable2Ds.Test26.d2_t_new.y at time=1? I believe it should be -8 (as is the case in MSL v4.0.0 and also sensible when loooking at d2_t_new.u) and not jump to zero.

I will investigate it further as there seems to be some problem for other cases, but I don't see that specific case as problematic: If you simulate to 2 s, you see that the derivative jumps to zero after 1 s. Thus both -8 and 0 seem acceptable at 1 s.

HansOlsson commented 4 months ago

The other case seems a lot more important; I was afraid that I had messed up something - but that doesn't seem like the case.

Consider the following two models:

package ProbTest
  model Test26Var
    extends ModelicaTest.Tables.CombiTable2Ds.Test26(t_new(extrapolation=Modelica.Blocks.Types.Extrapolation.HoldLastPoint));
    annotation (experiment(StopTime=2));
  end Test26Var;

  model tabletest3894
    Modelica.Blocks.Sources.Ramp ramp(
      height=1,
      duration=1,
      offset=1)
      annotation (Placement(transformation(extent={{-80,12},{-60,32}})));
    Modelica.Blocks.Sources.Constant
                                 const(k=-1)
      annotation (Placement(transformation(extent={{-80,-36},{-60,-16}})));
    Modelica.Blocks.Tables.CombiTable2Ds combiTable2Ds1(
      tableOnFile=false,
      table=[0,1,2; 1,2,3; 2,3,4],
      smoothness=Modelica.Blocks.Types.Smoothness.LinearSegments,
      extrapolation=Modelica.Blocks.Types.Extrapolation.HoldLastPoint)
      annotation (Placement(transformation(extent={{-14,-10},{6,10}})));
    Modelica.Blocks.Continuous.Derivative derivative(initType=Modelica.Blocks.Types.Init.SteadyState)
      annotation (Placement(transformation(extent={{32,-10},{52,10}})));
    Modelica.Blocks.Continuous.Der der1
      annotation (Placement(transformation(extent={{32,-40},{52,-20}})));
  equation 

    connect(combiTable2Ds1.u1, ramp.y) annotation (Line(points={{-16,6},{-50,6},{-50,
            22},{-59,22}},      color={0,0,127}));
    connect(combiTable2Ds1.u2,const. y) annotation (Line(points={{-16,-6},{-52,-6},
            {-52,-26},{-59,-26}}, color={0,0,127}));
    connect(combiTable2Ds1.y, derivative.u)
      annotation (Line(points={{7,0},{30,0}}, color={0,0,127}));
    connect(combiTable2Ds1.y, der1.u) annotation (Line(points={{7,0},{22,0},{22,
            -30},{30,-30}}, color={0,0,127}));
    annotation (
      Icon(coordinateSystem(preserveAspectRatio=false)),
      Diagram(coordinateSystem(preserveAspectRatio=false)),
      uses(Modelica(version="4.0.0")),
      experiment(StopTime=1, __Dymola_Algorithm="Dassl"),
      __Dymola_Commands(executeCall(
          ensureSimulated=true,
          autoRun=true) = {createPlot(
          id=1,
          position={0,0,1228,756},
          y={"ramp.y","ramp1.y"},
          range={0.0,1.2000000000000002,-2.0,20.0},
          grid=true,
          colors={{28,108,200},{238,46,47}},
          timeUnit="s"),createPlot(
          id=1,
          position={0,0,1228,756},
          y={"combiTable2Ds1.y","derivative.y","der1.y"},
          range={0.0,1.2000000000000002,-5.0,35.0},
          grid=true,
          subPlot=102,
          colors={{28,108,200},{0,140,72},{238,46,47}},
          timeUnit="s")} "plots"));
  end tabletest3894;
  annotation (uses(ModelicaTest(version="4.1.0"), Modelica(version="4.1.0")));
end ProbTest;

Currently tabletest3894 gives reasonable result, but Test26Var does after 1s have a constant signal with derivative -2. That is clearly non-sense, and blocking.

Manually reverting #3896 fixes Test26Var, but tabletest3894 then gives bad results.

beutlich commented 4 months ago

That is clearly non-sense, and blocking.

Thanks for finding. It is addressed by #4415.

HansOlsson commented 3 months ago

To do:

casella commented 3 months ago

@HansOlsson what about PR #4360? Should it also be merged or is it now obsolete? If it is obsolete, please close it with a short explanation for the record. Thanks!

@HansOlsson, @beutlich once #4415 is cherry-picked, who takes care of rebuilding the binaries?

beutlich commented 3 months ago

who takes care of rebuilding the binaries?

The library officers do.

beutlich commented 3 months ago

what about PR #4360?

This is controversial, see https://github.com/modelica/ModelicaStandardLibrary/pull/4360#issuecomment-2161123419

casella commented 3 months ago

@beutlich I'm sorry that I cannot really follow the discussion in #4360, as it is very technical and implementation-related. Could you provide a concrete use case or MWE that shows what the regression is all about?

Thanks

casella commented 3 months ago

who takes care of rebuilding the binaries?

The library officers do.

OK. Two questions about that:

Thanks!

beutlich commented 3 months ago

@beutlich I'm sorry that I cannot really follow the discussion in #4360, as it is very technical and implementation-related. Could you provide a concrete use case or MWE that shows what the regression is all about?

The example is all here in this issue. I am going to clarify with @HansOlsson such that there will be a common understanding. Thanks.

beutlich commented 3 months ago

do I understand correctly that the built libraries are made available directly in the Modelica\Resources\Library\ resource directory of the GIT repository.

The commit history gives the answer: https://github.com/modelica/ModelicaStandardLibrary/commits/master/Modelica/Resources/Library

will they be available on both the master and maint/4.1.x branches?

Since adding binaries to this repository has been controversial from the beginning (convenience vs. binary blobs in git) and binaries should not be part of the git history the behaviour will be changed in the future for master branch. Check the open PRs: https://github.com/modelica/ModelicaStandardLibrary/pulls?q=is%3Apr+is%3Aopen+binaries+label%3A%22L%3A+Resources%22+

casella commented 3 months ago

@beutlich I understand what is missing to close this ticket is

Do I miss something else?

beutlich commented 3 months ago

cherry pick

No, #4415 is already back-ported from master to maint/4.1.x via #4417. Hence, that's no more an issue.

Do I miss something else?

Yes, I am actually concerned about #4360 which I did not like to see closed. So far, no clarification.

rebuild the libraries

Can do. But I was waiting to see #4360 clarified first to avoid double work.

casella commented 3 months ago

I'm not sure #4360 can be considered a blocker for the whole 4.1.0 release. This seems to me a really minor issue. If you can find an agreement with @HansOlsson that would be good, otherwise I'd keep it open for 4.2.0.

Thanks!

maltelenz commented 3 months ago

I understand what is missing to close this ticket is

Do I miss something else?

Yes, the reference results need to be updated (once the binaries are rebuilt?), per earlier comments in this issue.

beutlich commented 3 months ago

This seems to me a really minor issue.

Not sure actually.

If you can find an agreement with @HansOlsson that would be good

Well, I was hoping so.

beutlich commented 3 months ago
  • rebuild the libraries and place them under Resources on maint/4.1.x

There now is #4426.