Skip to content

Conversation

@henrikt-ma
Copy link
Contributor

Since the Modelica.Blocks.Sources.Ramp block 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.

@henrikt-ma henrikt-ma added the L: Blocks Issue addresses Modelica.Blocks label Apr 24, 2023
@henrikt-ma henrikt-ma requested a review from HansOlsson April 24, 2023 09:59
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.

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.

@henrikt-ma
Copy link
Contributor Author

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 duration(min = Constants.small), as that is a required additional change for the PR to make sense. Now, the two changes can be considered jointly.

@henrikt-ma henrikt-ma changed the title Guarantee continuity of Ramp Make Ramp a continuous source Apr 24, 2023
@HansOlsson
Copy link
Contributor

(After some git annotate checking.)
The min=0 is due to #943
Thus I don't see that we should change it.

@henrikt-ma
Copy link
Contributor Author

The fact that there is an historical issue from 2012 behind a previous change from small to 0 doesn't mean it was a great decision to make the change. The old issue doesn't contain a lot of strong reasons for allowing the extreme case of a discontinuity, so I'm suggesting that we here and now revisit this old topic, this time with the consequence of never being able to ensure continuity in mind.

@henrikt-ma henrikt-ma requested a review from rfranke April 24, 2023 12:51
@henrikt-ma
Copy link
Contributor Author

Let's see what the original contributor @rfranke has to say.

@HansOlsson
Copy link
Contributor

Another option would be to make the smooth conditional, smooth(if duration>0 then 0 else -1, ...) (assuming it is defined), but that is likely to cause duration to be evaluated - which likely causes more problems.

@rfranke
Copy link
Contributor

rfranke commented Apr 24, 2023

This would break Modelica.Fluid.Examples.BranchingDynamicPipes that (mis?)uses Ramp as Step -- see #943. It currently specifies duration=0. How many more existing models would be affected?
Can you motivate with an example how smooth(0 would be superior over an event, i.e. trading discrete decisions for step size control?

@henrikt-ma
Copy link
Contributor Author

henrikt-ma commented Apr 25, 2023

Can you motivate with an example how smooth(0 would be superior over an event, i.e. trading discrete decisions for step size control?

No, because I expect good tools to take the opportunity to generate events also for expressions inside smooth(0, …). :)

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.

Copy link
Contributor

@rfranke rfranke left a 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?

@henrikt-ma
Copy link
Contributor Author

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

I am afraid that the complicated nature of this expression would make it unrealistic to expect tools to take advantage of the continuity guarantee.

Alternatively analyse the impact on existing libraries and at least fix it in this MSL.

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.

Don't know, how strict you are with semantic versioning. Would this then be MSL 5.0.0?

Good question. For me it is more important to see the direction of future development than getting a fix into the next release.

@henrikt-ma henrikt-ma marked this pull request as draft April 26, 2023 12:26
@henrikt-ma
Copy link
Contributor Author

As far as I can tell, BranchingDynamicPipes is the only model where a Ramp has zero duration. Removing Draft state.

@henrikt-ma henrikt-ma marked this pull request as ready for review April 26, 2023 14:11
@AHaumer
Copy link
Contributor

AHaumer commented Apr 27, 2023

To be honest, you don't know in how many user models the ramp is misused as a step.
Why shouldn't a tool figure out something like:

parameter SI.Time duration(min=0.0, start=2) "Duration of ramp (= 0.0 gives a Step)
  annotation(Evaluate=true)";

if duration < Constants.small then
  y = offset + (if time < startTime then 0 else height);
else
  y = offset + smooth(0, (if time < startTime then 0 
                                  elseif time < (startTime + duration) then (time - startTime)*height/duration else height));
endif;

What else would be the use of "smooth"?

@rfranke
Copy link
Contributor

rfranke commented Apr 27, 2023

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

@AHaumer
Copy link
Contributor

AHaumer commented Apr 27, 2023

"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"?

@henrikt-ma
Copy link
Contributor Author

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

Yes, and my small investigation didn't reveal any other cases than BranchingDynamicPipes. However, there are many Modelica libraries that are not on my radar, so I can't speak for the whole world of publicly available Modelica libraries, and of course I cannot at all speak for the hidden world of non-public models.

@henrikt-ma
Copy link
Contributor Author

To be honest, you don't know in how many user models the ramp is misused as a step. Why shouldn't a tool figure out something like:

parameter SI.Time duration(min=0.0, start=2) "Duration of ramp (= 0.0 gives a Step)
  annotation(Evaluate=true)";

if duration < Constants.small then
  y = offset + (if time < startTime then 0 else height);
else
  y = offset + smooth(0, (if time < startTime then 0 
                                  elseif time < (startTime + duration) then (time - startTime)*height/duration else height));
endif;

I would consider making duration an evaluated parameter a more annoying backward incompatibility than requiring it to be positive.

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 duration, but I would be surprised if tool vendors in general are ready to make the effort.

What else would be the use of "smooth"?

I don't get this question, please explain.

@HansOlsson
Copy link
Contributor

Since the Modelica.Blocks.Sources.Ramp block 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.

(I realize this a bit late - didn't look closely enough.)
Why would there be discontinuities at all?

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.

@henrikt-ma
Copy link
Contributor Author

Why would there be discontinuities at all?

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!

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 smooth(0, …) one can completely avoid re-evaluating the block output at the event and thereby obtain a truly continuous simulation result.

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

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?

@HansOlsson
Copy link
Contributor

Why would there be discontinuities at all?
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!

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.

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 startTime event, since if time=startTime then (time-startTime) should be 0.

For the end of duration event there are two issues: the first is that((startTime+duration)-startTime) is not necessarily duration (depends on a number of factors, it is often the case - e.g. if startTime=0 but not always), and the second is that duration*height/duration isn't necessarily height (in many cases it will be exact). Rewriting it as

y = offset + (if time < startTime then 0 else if time < (startTime +
      duration) then height*((time - startTime)/duration) else height);

should make it true before tools apply their internal rewriting, except for the first issue.

However, with a continuity guarantee from smooth(0, …) one can completely avoid re-evaluating the block output at the event and thereby obtain a truly continuous simulation result.

Note that smooth only guarantees smoothness if all "inputs" to the formula are smooth; thus it is not clear that adding it will necessarily solve the issue.

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?

When dealing with floating point numbers having a criteria adapted to floating point arithmetic should be the default, and not seen as cheating.

@rfranke
Copy link
Contributor

rfranke commented Apr 29, 2023

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?
modelica-3rdparty/ExternalMedia#12

Edit #2: even in MSL -- how did you check?!
https://github.com/modelica/ModelicaStandardLibrary/blob/44c8fe54cdf495dca0287ac981b82b73abc2b250/ModelicaTestOverdetermined.mo#L43C1-L47

@HansOlsson
Copy link
Contributor

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.

@HansOlsson
Copy link
Contributor

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.

@henrikt-ma
Copy link
Contributor Author

With so many other sources offering a continuous alternative (see #4150), it doesn't make sense that a ramp wouldn't be continuous.

One possible compromise could be to introduce this for Ramp:

    parameter Boolean continuous = true "Make output continuous by not allowing zero duration"
     annotation(Evaluate=true);

@HansOlsson
Copy link
Contributor

With so many other sources offering a continuous alternative (see #4150), it doesn't make sense that a ramp wouldn't be continuous.

They were all introduced to be backwards compatible, i.e. have continuous = false as default - even if the continuous variant was preferable.

One possible compromise could be to introduce this for Ramp:

    parameter Boolean continuous = true "Make output continuous by not allowing zero duration"
     annotation(Evaluate=true);

@henrikt-ma
Copy link
Contributor Author

They were all introduced to be backwards compatible, i.e. have continuous = false as default - even if the continuous variant was preferable.

The difference with the Ramp is that the continuous behavior is probably what most users will expect, and that the difference is barely noticeable. For the other sources, setting continuous = true is generally going to make a much bigger difference, which makes it much more relevant to ensure backwards compatibility by defaulting to continuous = false.

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.

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;

@henrikt-ma
Copy link
Contributor Author

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:

It's an interesting proposal. I have some questions:

  • Is the table implementation giving a guarantee that there won't be even tiny discontinuities at the ramp-in and ramp-out time events?
  • Isn't there a risk of unforeseen consequences for symbolic processing which previously had access to the ramp equations?

@HansOlsson
Copy link
Contributor

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:

Interesting idea.

However, to me this seems a bit heavy which poses two questions:

  • How efficient is this for models with many ramps?
  • How does this impact understandability of models? There's sometimes the complaints that "simple" Modelica models are too complicated for beginners.

@beutlich
Copy link
Member

beutlich commented Feb 9, 2026

Thanks for the open-minded responses.

Is the table implementation giving a guarantee that there won't be even tiny discontinuities at the ramp-in and ramp-out time events?

External C function ModelicaStandardTables_CombiTimeTable_nextTimeEvent uses an epsilon arithmetic to determine the next time event. And yes, I forgot about it, there always will be time events for ramp-in and ramp-out time points. Hence, the proposed model is not as valid as I thought to be.

Isn't there a risk of unforeseen consequences for symbolic processing which previously had access to the ramp equations?

First and second derivatives are provided by CombiTimeTable.

How efficient is this for models with many ramps?

I have no benchmarks, but I assume that external C functions ModelicaStandardTables_CombiTimeTable_getValue and ModelicaStandardTables_CombiTimeTable_nextTimeEvent have a higher cyclomatic complextity than the current generated ramp code from Modelica.

How does this impact understandability of models? There's sometimes the complaints that "simple" Modelica models are too complicated for beginners.

That's a valid point. The ramp model will be too complex for Dymola Trial version, so yes, it will break user expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L: Blocks Issue addresses Modelica.Blocks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants