-
Notifications
You must be signed in to change notification settings - Fork 180
Make Ramp a continuous source #4121
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
base: master
Are you sure you want to change the base?
Conversation
HansOlsson
left a comment
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.
At a first glance it seems like a good improvement, but unfortunately it is not always valid.
Looking at the variable declarations we have:
parameter SI.Time duration(min=0.0, start=2)
"Duration of ramp (= 0.0 gives a Step)";
so duration=0 and height>0 is a legal ramp, but not continuous - i.e. not smooth(0, ...)
I don't know if we have a test for that.
Good catch. I added |
|
(After some git annotate checking.) |
|
The fact that there is an historical issue from 2012 behind a previous change from |
|
Let's see what the original contributor @rfranke has to say. |
|
Another option would be to make the smooth conditional, |
|
This would break |
No, because I expect good tools to take the opportunity to generate events also for expressions inside For reference: https://specification.modelica.org/master/operators-and-expressions.html#modelica:smooth Edit: For a more general background of this issue, I am currently working on eliminating spurious discontinuities of simulation results that ought to be continuous. That something which is continuous has tiny discontinuities in the results may not affect simple applications such as just plotting the result, but is a nuisance for more advanced post-processing. Examples of more advanced post-processing include approximating the derivative, using the signal as a data source for another model, or performing high fidelity comparison of simulation results. |
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.
This breaks existing models, like Modelica.Fluid.Examples.BranchingDynamicPipes. Ideally the change should be extended to be backwards compatible, something like
y = if duration < Constants.small then
offset + (if time < startTime then 0 else if time < (startTime + duration)
then (time - startTime)*height/duration else height)
else
offset + smooth(0, (if time < startTime then 0 else if time < (startTime + duration)
then (time - startTime)*height/duration else height));Alternatively analyse the impact on existing libraries and at least fix it in this MSL.
Don't know, how strict you are with semantic versioning. Would this then be MSL 5.0.0?
I am afraid that the complicated nature of this expression would make it unrealistic to expect tools to take advantage of the continuity guarantee.
Sure, that would be required to make this PR complete. I'll change to Draft state while working out the MSL part. I am not the right person to judge the effect on other libraries.
Good question. For me it is more important to see the direction of future development than getting a fix into the next release. |
|
As far as I can tell, |
|
To be honest, you don't know in how many user models the ramp is misused as a step. What else would be the use of "smooth"? |
|
"good tools" run tests with many if not all available libraries regularly. @henrikt-ma it should be doable to introduce the change into your tests tonight, in order to get an idea. |
I was talking about user models, not publicly avialable libraries ... Why annoy such "quit users"? |
Yes, and my small investigation didn't reveal any other cases than |
I would consider making Apart from that, what we should expect of tools is of course a matter for discussion. From my perspective, I can see that we could handle this without evaluating
I don't get this question, please explain. |
(I realize this a bit late - didn't look closely enough.) The ramp should generate time-events, and ideally solvers should be able to exactly hit the point of the time-event and evaluate the expressions before and after, and they should be exactly the same and thus there shouldn't be any discontinuity! I checked in Dymola - and even if Dymola normally store two points at events (before and after) one of them is skipped for Modelica.Clocked.Examples.SimpleControlledDrive.Continuous (when using Dassl) since there is no change at the events (yes, I started the simulation at -1 s). Oviously there are the normal double points for Modelica.Fluid.Examples.BranchingDynamicPipes. |
Only in exact arithmetic. In reality, we use floating point computations, and one cannot expect numerical results to be identical on both sides of the event. However, with a continuity guarantee from
Considering what I explained above, maybe you were just lucky that the floating point computation resulted in the very same value as on the other side of the event, or did you cheat by ignoring tiny changes in the result? |
In general that is true, but one needs to consider each expression to see whether that is actually the case. E.g., one can expect that the values to be identical for the For the end of duration event there are two issues: the first is that should make it true before tools apply their internal rewriting, except for the first issue.
Note that
When dealing with floating point numbers having a criteria adapted to floating point arithmetic should be the default, and not seen as cheating. |
|
I think the change is fine if you have strong reasons and show by checking a large number of available model libraries that this doesn't harm. Edit: didn't you see this when saying that BranchingDynamicPipes is the only use? Maybe some conversion script or other user support needed? Edit #2: even in MSL -- how did you check?! |
|
I'm not sure that MSL is similar to the normal use, as almost all uses of Ramp in MSL are at the top-level for simple Examples. In normal libraries it is often more complicated (part of a sub-model or a base-class; and the parameter is not set directly but part of e.g., a parameter sweep) and those are exactly the cases where the extra flexibility of duration=0 is more beneficial. (However, it is often also a replaceable component so it could be redeclared if needed.) And unfortunately those advanced uses is not something a conversion script can handle. |
|
I realized that I recently actually took the limit of duration->0 and also tested duration=0 (so that they generate the same result) when investigating an issue for a user model. |
|
With so many other sources offering a One possible compromise could be to introduce this for |
They were all introduced to be backwards compatible, i.e. have
|
The difference with the |
beutlich
left a comment
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.
Here's my orthogonal proposal to make the time event handling explicit and still allow the limit behaviour of a step using the behaviour of CombiTimeTable. In case of duration = 0, a time event is generated, otherwise only of ramp-in and ramp-out if timeEvents is set to true:
block Ramp "Generate a (possibly discontinuous) ramp signal"
parameter Real height=1 "Height of ramp"
annotation(Dialog(groupImage="modelica://Modelica/Resources/Images/Blocks/Sources/Ramp.png"));
parameter SI.Time duration(min=0.0, start=2) "Duration of ramp (= 0.0 gives a Step)";
parameter Boolean timeEvents=true "Time event handling";
extends Interfaces.SignalSource;
protected
CombiTimeTable combiTimeTable(
final tableOnFile=false,
final table=[0, 0; duration, height],
final smoothness=Modelica.Blocks.Types.Smoothness.LinearSegments,
final extrapolation=Modelica.Blocks.Types.Extrapolation.LastTwoPoints,
final timeScale=1,
final offset={offset},
final startTime=startTime,
final timeEvents=if timeEvents then Modelica.Blocks.Types.TimeEvents.Always else Modelica.Blocks.Types.TimeEvents.AtDiscontinuities) annotation (Placement(transformation(extent={{-10,-10},{10,10}})));
equation
connect(combiTimeTable.y[1], y) annotation (Line(points={{11,0},{110,0}}, color={0,0,127}));
annotation (
Icon(coordinateSystem(
preserveAspectRatio=true,
extent={{-100,-100},{100,100}}), graphics={
Line(points={{-80,68},{-80,-80}}, color={192,192,192}),
Polygon(
points={{-80,90},{-88,68},{-72,68},{-80,90}},
lineColor={192,192,192},
fillColor={192,192,192},
fillPattern=FillPattern.Solid),
Line(points={{-90,-70},{82,-70}}, color={192,192,192}),
Polygon(
points={{90,-70},{68,-62},{68,-78},{90,-70}},
lineColor={192,192,192},
fillColor={192,192,192},
fillPattern=FillPattern.Solid),
Line(points={{-80,-70},{-40,-70},{31,38}}),
Text(
extent={{-150,-150},{150,-110}},
textString="duration=%duration"),
Line(points={{31,38},{86,38}})}),
Documentation(info="<html>
<p>
The Real output y is a ramp signal:
</p>
<div>
<img src=\"modelica://Modelica/Resources/Images/Blocks/Sources/Ramp.png\"
alt=\"Ramp.png\">
</div>
<p>
If parameter duration is set to 0.0, the limiting case of a Step signal is achieved.
</p>
</html>"));
end Ramp;
It's an interesting proposal. I have some questions:
|
Interesting idea. However, to me this seems a bit heavy which poses two questions:
|
|
Thanks for the open-minded responses.
External C function
First and second derivatives are provided by
I have no benchmarks, but I assume that external C functions
That's a valid point. The ramp model will be too complex for Dymola Trial version, so yes, it will break user expectations. |
Since the
Modelica.Blocks.Sources.Rampblock generates events when switching between the three parts of the ramp, it is more or less expected to get tiny discontinuities at the events. By giving a continuity guarantee, tools may decide to not update the value of the ramp output at the events, and thereby obtain a continuous output signal.