ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

Make Ramp a continuous source

Open henrikt-ma opened this issue 2 years ago • 23 comments

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 avatar Apr 24 '23 09:04 henrikt-ma

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 avatar Apr 24 '23 12:04 henrikt-ma

(After some git annotate checking.) The min=0 is due to https://github.com/modelica/ModelicaStandardLibrary/issues/943 Thus I don't see that we should change it.

HansOlsson avatar Apr 24 '23 12:04 HansOlsson

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 avatar Apr 24 '23 12:04 henrikt-ma

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

henrikt-ma avatar Apr 24 '23 12:04 henrikt-ma

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.

HansOlsson avatar Apr 24 '23 12:04 HansOlsson

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?

rfranke avatar Apr 24 '23 19:04 rfranke

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.

henrikt-ma avatar Apr 25 '23 11:04 henrikt-ma

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 avatar Apr 26 '23 12:04 henrikt-ma

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

henrikt-ma avatar Apr 26 '23 14:04 henrikt-ma

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

AHaumer avatar Apr 27 '23 16:04 AHaumer

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

rfranke avatar Apr 27 '23 17:04 rfranke

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

AHaumer avatar Apr 27 '23 17:04 AHaumer

"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 avatar Apr 28 '23 06:04 henrikt-ma

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.

henrikt-ma avatar Apr 28 '23 06:04 henrikt-ma

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.

HansOlsson avatar Apr 28 '23 07:04 HansOlsson

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?

henrikt-ma avatar Apr 28 '23 07:04 henrikt-ma

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.

HansOlsson avatar Apr 28 '23 09:04 HansOlsson

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? https://github.com/modelica-3rdparty/ExternalMedia/issues/12

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

rfranke avatar Apr 29 '23 07:04 rfranke

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 avatar Jun 22 '23 13:06 HansOlsson

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.

HansOlsson avatar Jun 30 '23 13:06 HansOlsson

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

henrikt-ma avatar Jun 30 '23 19:06 henrikt-ma

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

HansOlsson avatar Jul 03 '23 07:07 HansOlsson

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.

henrikt-ma avatar Jul 07 '23 18:07 henrikt-ma