MSL 4.1.0 Regressions - Blocks.Tables

[GallLeo]

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

Changed C-code/binaries, need reference update after library officer check:

• ModelicaTest.Tables.CombiTable2Ds.Test26
  - Reason: Change of edge case
  - Updated reference files? @GallLeo please update reference files once Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415 is cherry-picked
• ModelicaTest.Tables.CombiTable2Ds.Test27
  - Reason: Change of edge case
  - Updated reference files? @GallLeo please update reference files once Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415 is cherry-picked
• ModelicaTest.Tables.CombiTable2Dv.Test26
  - Reason: Change of edge case
  - Updated reference files? @GallLeo please update reference files once Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415 is cherry-picked
• ModelicaTest.Tables.CombiTable2Dv.Test27
  - Reason: Change of edge case
  - Updated reference files? @GallLeo please update reference files once Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415 is cherry-picked

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:

• ModelicaTest.Tables.CombiTable2Ds.Test15
  - Updated comparisonSignals.txt?
  - Updated reference files?
• ModelicaTest.Tables.CombiTable2Ds.Test16
  - Updated comparisonSignals.txt?
  - Updated reference files?
• ModelicaTest.Tables.CombiTable2Ds.Test17
  - Updated comparisonSignals.txt?
  - Updated reference files?

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

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

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]

Tested revision: f9bddf8 (2024-02-16)

f9bddf8 is not part of v4.1.0-beta.1. Should it be da5c5de?

[beutlich]

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]

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

ModelicaStandardLibrary/Modelica/Resources/C-Sources/ModelicaStandardTables.c

Lines 47 to 56 in da5c5de

	Jan. 31, 2022: by Hans Olsson, Dassault Systemes
	Added better support for one-sided derivatives of CombiTable2D.
	The idea is that when we are computing the derivative at a boundary
	in the table we should consider the der-value to choose side.
	This is less important for 1d-tables and thus ignored in those cases.
	(ticket #3893)
	Nov. 12, 2021: by Thomas Beutlich
	Fixed derivatives in CombiTable2D for one-sided extrapolation
	by constant continuation (ticket #3894)

[MatthiasBSchaefer]

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]

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]

@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

[beutlich]

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]

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.

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

[beutlich]

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]

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

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

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]

That is clearly non-sense, and blocking.

Thanks for finding. It is addressed by #4415.

[HansOlsson]

To do:

• Just update reference results for ModelicaTest.Tables.CombiTable2D[sv].Test2[67]
• The other one:
  • Merge Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415
  • Cherry pick Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415 to maint/4.1.x done in PR Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4417.
  • Rebuild ModelicaStandardLibraries and promote on maint/4.1.x

[casella]

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

who takes care of rebuilding the binaries?

The library officers do.

[beutlich]

what about PR #4360?

This is controversial, see #4360 (comment)

[casella]

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

who takes care of rebuilding the binaries?

The library officers do.

OK. Two questions about that:

• do I understand correctly that the built libraries are made available directly in the Modelica\Resources\Library\ resource directory of the GIT repository?
• will they be available on both the master and maint/4.1.x branches?

Thanks!

[beutlich]

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

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]

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

• cherry pick Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415 to maint/4.1.x
• rebuild the libraries and place them under Resources on maint/4.1.x (not master, that will be another story)

Do I miss something else?

[beutlich]

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]

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]

I understand what is missing to close this ticket is

• cherry pick Restore derivatives for one-sided extrapolation by constant continuation in case of 2D tables that actually degrade to 1D tables #4415 to maint/4.1.x
• rebuild the libraries and place them under Resources on maint/4.1.x (not master, that will be another story)

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]

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]

• rebuild the libraries and place them under Resources on maint/4.1.x

There now is #4426.