Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify documentation on periodic table extrapolation #4287

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

paultjevdh
Copy link

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.

@beutlich beutlich added L: Blocks Issue addresses Modelica.Blocks documentation Issue addresses the documentation labels Jan 28, 2024
@beutlich beutlich linked an issue Jan 28, 2024 that may be closed by this pull request
@beutlich beutlich changed the title Clarified documentation on periodic table extrapolation. Clarify documentation on periodic table extrapolation Jan 28, 2024
The code changes were generated in a modelica program which uses the <b> tag for bold. This is apparently deprecated in github, thus the code was fixed by substituting with <strong>.
@CLAassistant
Copy link

CLAassistant commented Jan 29, 2024

CLA assistant check
All committers have signed the CLA.

Modelica/Blocks/Tables.mo Outdated Show resolved Hide resolved
Modelica/Blocks/Tables.mo Outdated Show resolved Hide resolved
Modelica/Blocks/Tables.mo Outdated Show resolved Hide resolved
Modelica/Blocks/Tables.mo Outdated Show resolved Hide resolved
Modelica/Blocks/Tables.mo Outdated Show resolved Hide resolved
Modelica/Blocks/Tables.mo Outdated Show resolved Hide resolved
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will wait with review until CLA-status is cleared and other comments are handled.

paultjevdh and others added 3 commits January 29, 2024 11:00
Fixed tab/space indentation issue

Co-authored-by: tobolar <[email protected]>
Fixed tab/space indentation issue

Co-authored-by: tobolar <[email protected]>
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'd consider a periodic function even correct if first and last data values are not identical, that is, the function is discontinuous at the interval boundary. This is legal and by design, even if it might be undesired. There are no explicit boundary conditions.

@paultjevdh
Copy link
Author

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

@beutlich
Copy link
Member

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
Copy link
Author

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
Copy link
Member

beutlich commented Feb 2, 2024

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

OK. Confirmed.

@paultjevdh
Copy link
Author

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

@tobolar tobolar requested a review from HansOlsson March 25, 2024 08:53
Modelica/Blocks/Tables.mo Outdated Show resolved Hide resolved
Changed the documentation to recognize that multiple outputs are possible
Changed documentation to reflect that multiple inputs and outputs are possible.
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced that these changes are correct. My comments from #4287 (review) and #4287 (comment) still hold.

Instead, it might be desirable to add a new example model ot test model, where we have three table blocks with C1 interpolation and periodic extrapolation and

  1. getValue(u_max) != getValue(u_min)
  2. getValue(u_max) == getValue(u_min) and getDerValue(u_max) != getDerValue(u_min)
  3. getValue(u_max) == getValue(u_min) and getDerValue(u_max) == getDerValue(u_min)

The proposed comments can then go tho the documentation of this model.

@paultjevdh
Copy link
Author

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.

  • For simulation/integration of a model, a requirement is that the integrand is continuous. Discontinuities require state events for correct simulation. The current implementation of the tables lacks this state event.
  • Higher order interpolation code (which use multiple table-points) should be aware of the discontinuity as well, which I believe is currently not the case.
    Thus, while discontinuous functions are conceivable, they will not be simulated correctly.

@paultjevdh
Copy link
Author

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
Copy link
Member

beutlich commented Apr 8, 2024

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
Copy link
Author

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:

  • why are there quotes around the block names?
  • The space in 'C1 continuous' does not work in omedit. It simulates, but there are no checkboxes in the plotting view.

@beutlich
Copy link
Member

  • 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
Copy link
Author

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

Simplified documentation in table models with reference to example.
@paultjevdh
Copy link
Author

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

Simplified documentation in table models with reference to example.
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still seems ok, I notice that the extra text about duplicated values is now gone making it cleaner.

@paultjevdh
Copy link
Author

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

Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not reviewing earlier... busy days.

I am not fully in line with the proposed changes and will update later ths month.

@beutlich beutlich self-requested a review May 21, 2024 16:30
@beutlich
Copy link
Member

beutlich commented May 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue addresses the documentation L: Blocks Issue addresses Modelica.Blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2D table extrapolation documentation clarification request
5 participants