Skip to content
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

Issues with Modelica.Blocks.Math.ContinuousMean #3906

Closed
HansOlsson opened this issue Nov 29, 2021 · 5 comments · Fixed by #4151
Closed

Issues with Modelica.Blocks.Math.ContinuousMean #3906

HansOlsson opened this issue Nov 29, 2021 · 5 comments · Fixed by #4151
Assignees
Labels
L: Blocks Issue addresses Modelica.Blocks
Milestone

Comments

@HansOlsson
Copy link
Contributor

I see two problems:

der(mu) = noEvent(if time >= t_0 + t_eps then (u-mu)/(time-t_0) else 0);
y = noEvent(if time >= t_0 + t_eps then mu else u);

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 HansOlsson added the L: Blocks Issue addresses Modelica.Blocks label Nov 29, 2021
@beutlich
Copy link
Member

beutlich commented Dec 1, 2021

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

@tobolar
Copy link
Contributor

tobolar commented Dec 1, 2021

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

@christiankral
Copy link
Contributor

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

@henrikt-ma
Copy link
Contributor

henrikt-ma commented Jan 21, 2022

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

@HansOlsson
Copy link
Contributor Author

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 added a commit to HansOlsson/Modelica that referenced this issue Jun 13, 2023
@beutlich beutlich added this to the MSL4.1.0 milestone Nov 14, 2023
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 a pull request may close this issue.

5 participants