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
452 stars 165 forks source link

Clarify documentation on periodic table extrapolation #4287

Open paultjevdh opened 5 months ago

paultjevdh commented 5 months ago

As discussed in #3793 , I updated the documentation for periodic extrapolation of all 1- and 2D tables in a way which I feel will prevent possible misinterpretations.

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

paultjevdh commented 5 months ago

I think mathematicians would not agree with you on periodicity in that case, as argued in #3793.

beutlich commented 5 months ago

Periodicity and continuity (or say differentiability more generally) at the interval boundary are orthogonal concepts. Even if claiming that first and last sample point should be identical for a "correct" interpolation, it still can be incorrect when using C1 spline interpolants in case there are no natural boundardy conditions. In that sense, nothing is promised and nothing can be assumed at the interval boundary.

In case of better diagnostics I might consider to add a one-time warning if first and last point are not identical for C0 interpolants and (also for derivative values in case of C1 interpolants).

paultjevdh commented 5 months ago

To clarify my last comment: I did not mean that discontinuous functions can't be periodic mathematically, so I agree that those concepts are orhogonal. A square wave or sawtooth are examples. However, those require care when defining the value at the discontinuity. What I meant to say was that the example sawtooth in modelica (in #3793) is not periodic in a mathematical sense because the definition in the discontinuity differs along the way.

I do not believe any direct action is required from my side on this, is that correct?

beutlich commented 5 months ago

I do not believe any direct action is required from my side on this, is that correct?

OK. Confirmed.

paultjevdh commented 3 months ago

It's been very silent here. Are there remaining issues to be resolved before this is accepted?

paultjevdh commented 3 months ago

I would love to make such a model if only I knew what C1 (or C0) interpolation means....

I agree that the documentation needs improvement to indicate that the function in the table does not need to be continuous. However, I believe that the current implementation of the table library is lacking some features which makes discontinuous periodic functions in the table flawed.

paultjevdh commented 3 months ago

I constructed a model with the 3 tables as you proposed. The model included contains 3 tables. 1: sawtooth, discontinuous at interval edge, table=[0, -1; 1, 1] 2: triangle (blue), starting at peak, slope is discontinuous at boundary, table=[0, -1; 1, 0; 2, 1; 3, 0; 4, -1] 3: triangle (green), stating at 0, continuous slope at interval boundary, table=[0, 0; 1, 1; 2, 0; 3, -1; 4, 0] Tables1Ds.zip The output is as follows (with Akima interpolation) image Is this what you meant as example and illustration for the accompanying documentation?

beutlich commented 3 months ago

I would love to make such a model if only I knew what C1 (or C0) interpolation means....

Cn continuity means that the n-th derivatives are continous at the intersection points. For interpolation the intersection points are the interval boundaries, for peridoc extrapolation the intersection point are the first and last abscissa (u_min and u_max).

I agree that periodic extrapolation is special (as it may lead to missing C0/C1 continuity at the intersection even if the interpolant itself has higher continuity behaviour). But it's all by design. See again section 3.2 of

Thomas Beutlich, Gerd Kurzbach and Uwe Schnabel. Remarks on the Implementation of the Modelica Standard Tables. In: Proceedings of the 10th International Modelica Conference. Ed. by Hubertus Tummescheit and Karl-Erik Årzén. Lund, Sweden, March 2014. DOI: 10.3384/ecp14096893.

Is this what you meant as example and illustration for the accompanying documentation?

Yes, that's what I meant. All three curves are valid by design. They could go to a new example model, say Modelica.Blocks.Examples.ContinuityPeriodicTableExtrapolation. I adapted your model w.r.t. naming of the blocks, a common period and Makima interpolation:

model ContinuityPeriodicTableExtrapolation "Compare continuity of period extrapolation of CombiTable1Ds/CombiTable1Dv"
  extends Modelica.Icons.Example;
  Modelica.Blocks.Sources.Ramp ramp(
    height=10,
    offset=-3,
    duration=1)
    annotation (Placement(transformation(origin={-18,0}, extent={{-10,-10},{10,10}})));
  Modelica.Blocks.Tables.CombiTable1Ds discontinuousExtrapol(
    table=[0,-1; 4,1],
    extrapolation=Modelica.Blocks.Types.Extrapolation.Periodic,
    smoothness=Modelica.Blocks.Types.Smoothness.ModifiedContinuousDerivative) "Table block with discontinuous periodic extrapolation"
    annotation (Placement(transformation(origin={22,30}, extent={{-10,-10},{10,10}})));
  Modelica.Blocks.Tables.CombiTable1Ds continuousC0ExtraPol(
    table=[0,-1; 1,0; 2,1; 3,0; 4,-1],
    extrapolation=Modelica.Blocks.Types.Extrapolation.Periodic,
    smoothness=Modelica.Blocks.Types.Smoothness.ModifiedContinuousDerivative)  "Table block with C0 periodic extrapolation"
    annotation (Placement(transformation(extent={{12,-10},{32,10}})));
  Modelica.Blocks.Tables.CombiTable1Ds continuousC1Extrapol(
    table=[0,-1; 0.25,-1; 0.5,-1; 2,1; 3.5,-1; 3.75,-1; 4,-1],
    extrapolation=Modelica.Blocks.Types.Extrapolation.Periodic,
    smoothness=Modelica.Blocks.Types.Smoothness.ModifiedContinuousDerivative) "Table block with C1 periodic extrapolation"
    annotation (Placement(transformation(origin={22,-30}, extent={{-10,-10},{10,10}})));
equation 
  connect(continuousC0ExtraPol.u, ramp.y)
    annotation (Line(points={{10,0},{-7,0}}, color={0,0,127}));
  connect(discontinuousExtrapol.u, ramp.y)
    annotation (Line(points={{10,30},{0,30},{0,0},{-7,0}}, color={0,0,127}));
  connect(continuousC1Extrapol.u, ramp.y)
    annotation (Line(points={{10,-30},{0,-30},{0,0},{-7,0}}, color={0,0,127}));
annotation (
  experiment(StartTime=0, StopTime=1, Tolerance=1e-6, Interval=0.01),
    Diagram(coordinateSystem(extent={{-60,-60},{60,60}})),
    Icon(coordinateSystem(extent={{-60,-60},{60,60}})));
end ContinuityPeriodicTableExtrapolation;
paultjevdh commented 2 months ago

So the fix is now a 2-part affair, where the technical nitty-gritty is handled by an example model. For the documentation of the tables themselves, I propose to simplify the text to = 3: Periodically repeat the table data (periodical function). Because no assumption can be made about the spacing of the samples defined in the first column, the repetition period is table[end,1]-table[1,1]. See ... in the examples.

Questions/remarks on the model:

beutlich commented 2 months ago
  • why are there quotes around the block names?

Because I needed to do so when aiming for blanks.

The space in 'C1 continuous' does not work in omedit

Seems like a tool issue. Single quoted identifiers ared tested by ModelicaCompliance.Components.Declarations.QuotedIdentifiers.

Anyway, I updated the above example and got rid of these single quoted idents.

paultjevdh commented 2 months ago

@beutlich I added your demonstration model with some explanation as to what it demonstrates to the examples section. I also re-edited the documentation in the table blocks. It now provides less explanation, instead it refers to the example.

paultjevdh commented 2 months ago

This should be more or less the final version, but the checks won't run automatically. Please advice.

paultjevdh commented 2 months ago

@HansOlsson Thanks. I'm a bit worried that this PR is in limbo on github though. 4 technical checks seem to be hanging, image or haven't these started yet? Also @beutlich does not seem to be alerted for this request.

beutlich commented 1 month ago

I already pushed some minor changes - but more work on wording and the missing comparisonSignals.txt is required. Will do later.