-
Notifications
You must be signed in to change notification settings - Fork 168
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
base: master
Are you sure you want to change the base?
Clarify documentation on periodic table extrapolation #4287
Conversation
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>.
There was a problem hiding this 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.
Fixed tab/space indentation issue Co-authored-by: tobolar <[email protected]>
Fixed tab/space indentation issue Co-authored-by: tobolar <[email protected]>
There was a problem hiding this 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.
I think mathematicians would not agree with you on periodicity in that case, as argued in #3793. |
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). |
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. I do not believe any direct action is required from my side on this, is that correct? |
OK. Confirmed. |
It's been very silent here. Are there remaining issues to be resolved before this is accepted? |
Clarified periodicity requirement
Changed the documentation to recognize that multiple outputs are possible
Changed documentation to reflect that multiple inputs and outputs are possible.
There was a problem hiding this 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
- getValue(u_max) != getValue(u_min)
- getValue(u_max) == getValue(u_min) and getDerValue(u_max) != getDerValue(u_min)
- 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.
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.
|
I constructed a model with the 3 tables as you proposed. |
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 ( 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
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; |
So the fix is now a 2-part affair, where the technical nitty-gritty is handled by an example model. Questions/remarks on the model:
|
Because I needed to do so when aiming for blanks.
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. |
@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.
2e8e966
to
fce8a3d
Compare
This should be more or less the final version, but the checks won't run automatically. |
Simplified documentation in table models with reference to example.
…m/paultjevdh/ModelicaStandardLibrary into Table-extrapolation_documentation
There was a problem hiding this 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.
@HansOlsson Thanks. I'm a bit worried that this PR is in limbo on github though. 4 technical checks seem to be hanging, |
There was a problem hiding this 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.
I already pushed some minor changes - but more work on wording and the missing comparisonSignals.txt is required. Will do later. |
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.