ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

Issues with Modelica.Blocks.Math.ContinuousMean

Open HansOlsson opened this issue 3 years ago • 5 comments

I see two problems:

  • It is not possible to set another startTime as for most blocks, see https://stackoverflow.com/questions/70124980/dymola-modelica-how-can-i-calculate-the-mean-of-a-signal-only-in-the-steady-st/70127878 (a previous issue was that startTime had no effect).
  • It is using an odd formula - without any explanation. I would guess it is for performance reasons, but I don't see that it actually gains anything. It might also be to gain precision - see comments at the end. The downside of the formulation is that it has a singularity that will mess with integrators, even if worked around.

https://github.com/modelica/ModelicaStandardLibrary/blob/8d090810980e1e2e51559721cdd4c267a4c849ae/Modelica/Blocks/Math.mo#L2318-L2319

I would propose the simpler:

  parameter Real startTime=0;
protected 
  Real mu(start=0, fixed=true) "Internal integrator variable";
equation 
  der(mu) = if time>=startTime then u else 0;
  y       = noEvent(if time > startTime + t_eps then mu/(time-startTime) else u);

Basically just integrating the signal and dividing by time. Note that we can safely skip "t_eps" with this formulation due to the subtle change.

If we for some reason really need the extra precision I would propose switching between the two formulas after some time, i.e. use the simple formula for the first part and then switch to the current formula.

HansOlsson avatar Nov 29 '21 13:11 HansOlsson

W.r.t. units the proposed change LGTM. Should mu and startTime be of type Modelica.Units.SI.Time?

beutlich avatar Dec 01 '21 06:12 beutlich

mu shall be of type Real, so that the unit can be deduced depending on y.

tobolar avatar Dec 01 '21 10:12 tobolar

I am in favor of the proposed change for the sake of having startTime consistently implemented.

I would yet like to comment that this change could cause a non-backwards compatible effect. If a simulation model of user has a simulation start time less than zero, the new implementation of the ContinuousMean model would behave differently as the start time is 0 by default but it was time at the initial time previously.

We might overcome this (unlikely case) by

  1. parameter SI.Time startTime = -Modelica.Constant.inf; // Which I do not like
  2. Describe the possible effect in the documentation

christiankral avatar Jan 20 '22 16:01 christiankral

I would yet like to comment that this change could cause a non-backwards compatible effect. If a simulation model of user has a simulation start time less than zero, the new implementation of the ContinuousMean model would behave differently as the start time is 0 by default but it was time at the initial time previously.

To me it would seem reasonable to try to restore this behavior rather than documenting the degraded functionality as a known potential issue.

(But I don't see exactly how initializing with -Modelica.Constant.inf could solve the problem. To me, it looks like this would just make der(mu) more or less zero.)

henrikt-ma avatar Jan 21 '22 08:01 henrikt-ma

I am in favor of the proposed change for the sake of having startTime consistently implemented.

I would yet like to comment that this change could cause a non-backwards compatible effect. If a simulation model of user has a simulation start time less than zero, the new implementation of the ContinuousMean model would behave differently as the start time is 0 by default but it was time at the initial time previously.

Ok, that is tricky - I assume something like the following will work:

  parameter Real startTime=-Modelica.Constants.inf;
protected 
  parameter Real t_0(fixed=false);
  Real mu(start=0, fixed=true) "Internal integrator variable";
  parameter Real actualStartTime=max(t_0, startTime);
initial equation
  t_0=time;
equation 
  der(mu) = if time>=actualStartTime then u else 0;
  y       = noEvent(if time > actualStartTime  then mu/(time-actualStartTime) else u);

(Or possibly having actualStartTime with fixed=false as well, might be more readable).

In general I see two problems with sources:

  • They have startTime=0 so we get this problem with starting before time=0 everywhere, e.g., when trying to test it with sine-source I stumbled on the same issue with the sine-source.
  • Many/most have a discontinuity at startTime - that is problematic; I noted sinc as a particular bad example.

HansOlsson avatar Jan 21 '22 09:01 HansOlsson