MassWithStopAndFriction might be numerically fragile in when statement

[bilderbuchi]

In an OM issue, @casella pointed out that the pattern

   stop = if s >= 1 then 1 else 0;
   when stop > 0 then
     reinit(s, 1);
   end when;

seems a bit fragile, because the conditional expression and the when statement are conceptually triggered simultaneously, with the second potentially affecting the first because it changes the value of s to be exactly the triggering value of the conditional expression. He might be wrong, but is seems a bit fishy, and potentially ambiguous and/or depending on tiny rounding errors.

(We are seeing issues with FMUs generated with such code, which is the topic of the above report.)

This is also used by MassWithStopAndFriction: https://github.com/modelica/ModelicaStandardLibrary/blob/a877f527e29c47e344d2c05e393b65c6c4eee01b/Modelica/Mechanics/Translational/Components/MassWithStopAndFriction.mo#L140-L145

and Franceso suggests using when pre(stopped) <> 0 then instead, as

In this case the reinit() statement is executed at the second event iteration, when it is no longer possible to influence the fact that stop becomes true, so there is no risk to trigger an infinite loop.

Possibly to be included in MSL 4.1.0. Thoughts?

[HansOlsson]

In an OM issue, @casella pointed out that the pattern

   stop = if s >= 1 then 1 else 0;
   when stop > 0 then
     reinit(s, 1);
   end when;

seems a bit fragile, because the conditional expression and the when statement are conceptually triggered simultaneously, with the second potentially affecting the first because it changes the value of s to be exactly the triggering value of the conditional expression. He might be wrong, but is seems a bit fishy, and potentially ambiguous and/or depending on tiny rounding errors.

Note that they are not fully conceptually done simultaneously; the reinit is by definition done after the rest of the model evaluation in a step in the event iteration.

Additionally that isn't an issue at all, due to the triggering requirements - as a state will change continuously and should guarantee that s is >=1 when triggering the when-clause.

However, the given model is broken:

• The reinit is sort of redundant, as s must be state and continuously change - and then reinit just do an epsilon-change; I'm not sure what the modeler intended.
• In case the derivative of s becomes negative, then s>0 it is not necessarily guaranteed to become false in the future, and thus even it may not necessarily become true again - that is fragile (it will work likely unless you add noEvent around if s >= 1 then 1 else 0). If stop is 0 or 1 as in this case use stop > 0.5 and not stop > 0 when checking its value.

(We are seeing issues with FMUs generated with such code, which is the topic of the above report.)

This is also used by MassWithStopAndFriction:

https://github.com/modelica/ModelicaStandardLibrary/blob/a877f527e29c47e344d2c05e393b65c6c4eee01b/Modelica/Mechanics/Translational/Components/MassWithStopAndFriction.mo#L140-L145

and Franceso suggests using when pre(stopped) <> 0 then instead, as

That would be an unneeded change possibly causing regressions, since it will delay the change.

Possibly to be included in MSL 4.1.0. Thoughts?

Absolutely not; the deadline has already passed (apart from the fact that the change is bad).

[bilderbuchi]

Thanks for your input, Hans!

• The reinit is sort of redundant, as s must be state and continuously change - and then reinit just do an epsilon-change; I'm not sure what the modeler intended.

I tried to follow git blame for MassWithStopAndFriction but got lost in several restructuring commits and their reverts, but it seems @christiankral @AHaumer might have an idea why it's implemented like it is?

Absolutely not; the deadline has already passed (apart from the fact that the change is bad).

Hey, ok. I'm just the messenger, here. :-)

[HansOlsson]

For the translational stop the velocity is also set to zero (so that it immediately stops moving) - and the state is changed to locked (so that it will not accelerate); and in that case an epsilon-change, even if almost redundant, might have some minor advantage.

[HansOlsson]

Oh, and a minor remark - the fragility of s>0 (and suggestion to use s>0.5) is only an issue if s is a Real, if s is an Integer (as in the stop-model) then it is fine; which is good as rewriting when stopped<>0 as when {stopped<-0.5,stopped>0.5} would be messy and confusing.

[HansOlsson]

Absolutely not; the deadline has already passed (apart from the fact that the change is bad).

Hey, ok. I'm just the messenger, here. :-)

No worries! And it wasn't so much regarding this specific issue as a general comment to stop the constant feature creep for the release.

[casella]

@HansOlsson, @bilderbuchi, some comments on my side

Note that they are not fully conceptually done simultaneously; the reinit is by definition done after the rest of the model evaluation in a step in the event iteration.

You are right, I missed this detail, as stated in MLS 8.3.6

reinit does not break the single assignment rule, because reinit(x, expr) in equations evaluates expr to a value, then at the end of the current event iteration step it assigns this value to x (this copying from values to reinitialized state(s) is done after all other evaluations of the model and before copying x to pre(x)).

The reinit is sort of redundant, as s must be state and continuously change - and then reinit just do an epsilon-change; I'm not sure what the modeler intended.

Neither am I. I guess the intent was to make sure that s has the exact value of 1 when stopped, rather than some approximated value due to the numerical handling of the if s >=1 condition. Ditto for the MSL component.

If stop is 0 or 1 as in this case use stop > 0.5 and not stop > 0 when checking its value.

stop is an Integer, so I thing stop > 0 (and stopped <> 0 in the MSL model) should be fine.

Francesco suggests using when pre(stopped) <> 0 then instead

That would be an unneeded change possibly causing regressions, since it will delay the change.

My intent was to make use of event iterations to handle the process: first the stop/stopped variable is changed when the limits are hit, and then, during the next event iteration, the consequence of that (i.e. the reinits) are handled. But it turns out this is not really necessary, because the reinit() semantics actually guarantees that behaviour without the need of event iterations.

And it wasn't so much regarding this specific issue as a general comment to stop the constant feature creep for the release.

I want to reassure @HansOlsson that I'm not trying to creep in any new features, only trying to remove bugs 😃. My definition of bug for the MSL include fishy models who may or may not work depending on tool-specific interpretations of what the model semantics is. As it turns out there is no bug here (because reinit() is actually applied at the end of the event iteration), there's no need to change the MSL.