-
Notifications
You must be signed in to change notification settings - Fork 44
Describe 'delay' as event-generating operator #3730
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
base: master
Are you sure you want to change the base?
Conversation
@henrikt-ma You looking for an implementation in other tools: in SimulationX both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine for me.
For me there are two different use cases:
I don't see a need to generalize spatialDistribution to non-Reals yet; but I see that the second case also applies for spatialDistribution. The issues with events for Reals as I see it are:
If we later refine the delay-operator's description we might revisit this. |
While I don't have a test implementation for this, I don't see that it would be more difficult than supporting it for |
If the
Yes, so it is important that a tool doesn't forget to support
It was not my intention that the text should be interpreted this way. The way I see it, the key change here is that |
Since both you and @HansOlsson bring up the non- |
My experience with handling spatialDistribution is that it is already quite complicated to get a good result; and since I see no real use case of non-Real spatialDistribution I think that will be an excessive burden for something for no real gain. |
I just realized that |
For me there are some issues:
Since it is a trivial to write cases where generating events for discrete variables will cause an infinite explosion in smaller and smaller discontinuities, I think we should be careful and not add something "just in case".
I can understand that delay of a non-Real needs to be a discrete-time expression (even if the delayTime is continuous-time) to not generate non-discrete-time Integer (etc), but I don't see a similar need for Reals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As indicated should not require an event for Reals, and support non-Real.
Could change to "next significant discontinuous change" and saying that changes for non-Reals are always significant.
|
||
These expression for \lstinline!div!, \lstinline!ceil!, \lstinline!floor!, and \lstinline!integer! are event generating expression. | ||
The event generating expression for \lstinline!mod(x,y)! is \lstinline!floor(x/y)!, and for \lstinline!rem(x,y)! it is \lstinline!div(x,y)! -- i.e., events are not generated when \lstinline!mod! or \lstinline!rem! changes continuously in an interval, but when they change discontinuously from one interval to the next. | ||
The event generating expression for \lstinline!delay! is the time remaining until the next discontinuity in the operator value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event generating expression for \lstinline!delay! is the time remaining until the next discontinuity in the operator value. | |
The event generating expression for \lstinline!delay! is the time remaining until the next significant discontinuous change in the operator value.' | |
If the delayed expression is non-Real the change is always significant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would miss the point. When there is a discontinuous change in the operator value, no matter how small, time must have been stopped for an event. Instead, note that the current text is not stressing that even the tiniest discontinuity in expr must be preserved -- this is up to tools to interpret intelligently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would miss the point. When there is a discontinuous change in the operator value, no matter how small, time must have been stopped for an event. Instead, note that the current text is not stressing that even the tiniest discontinuity in expr must be preserved -- this is up to tools to interpret intelligently.
I don't fully follow this:
Clearly the discontinuous change of the expression to be delayed happened during an event.
However, to me "discontinuous change in the operator value" mean a discontinuous change in the result of delay-operator (after the delay), which requires that an event was triggered.
If "discontinuous change" is intended to be interpreted somewhat loosely, I would say that it isn't clear at the moment.
And I would say that adding "significant" and stating that changes of discrete-valued types clarifies that intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now added a paragraph about event generation, and it includes significant in a statement about quality of implementation. OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I would say all of them should be automatically vectorized, and see also: #3728 |
How about expressing the value simply as |
This is not a good reason to not let
But this is not the way it will work for non- |
Interesting question - it's more complicated, at least Dymola seem to x.start for delay(x, 1) - and not the value of x at the start-time. I think that should be discussed separately. |
I am just extrapolating from what I learnt when working on |
OK. System Modeler is also making the (a little bit stretched) interpretation that Edit: Reported this as #3733. |
Discrete-time Integer and discrete-time Real variables are quite different, and thus many Real expression that only change at events are not formally discrete-time.
Additionally, I thought we wanted Added So a Real expression are often sort of discrete-time without being formally discrete-time; that's not the case for Integer expressions. |
Yes, absolutely! The variability of
I know this is the case, but to me there is only one natural way of defining the variability semantics of |
To me the important part for Real expressions is that discontinuities are propagated (with an event that cannot be observed internally), not that the call is discrete. |
Oh, so you mean that we should state more clearly that discontinuities should be preserved in the non-discrete-time case? Clarifying that wouldn't sound like a bad idea to me. |
Also OMC reproduces events that were present at the input of
To me this was the primary motivation to add events to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
\item | ||
\lstinline!delay($x$, $\ldots$)! where $x$ is a parameter expression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need delay of a parameter expression to be a parameter expression, and especially have an exception so that delay(p, 1+time/2)
is a parameter expression despite the time-argument?
As a user or developer I would wonder what the practical applications are of this; and I simply don't see any cases where users want to delay a parameter-expression.
Note that if we add some variant of more advanced initialization as in #3733 (comment) this will no longer hold; even when all of the arguments are parameter-expressions.
A simpler alternative is to add delay to the same exception-list as initial, terminal, etc (since delay has an implicit time-dependency).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For delay I see two sets of reasons to not add the parameter-rule for variability (but the rest of the PR is good).
- When understanding
delay
it makes sense to view it as havingtime
as implicit argument, and the explanation even that makes that clear by writing that it evaluates toexpr(time-timeDelay)
.
For fixed time-delays we can even compare it to spatialDistribution
where we have to use an explicit dependency:
model CC3
extends Modelica.Mechanics.Rotational.Examples.CoupledClutches;
Real J1_w_del=delay(J1.w, 0.1);
Real J1_w_sp1, J1_w_sp2=spatialDistribution(0 , J1.w, -time/0.1, false, {0,1},{10,10});
equation
(,J1_w_sp1)=spatialDistribution(J1.w, 0, time/0.1, true, {0,1},{10,10});
annotation (uses(Modelica(version="4.1.0")));
end CC3;
Here the three delayed signals are basically all the same, and it is just syntactic sugar that has removed time
from the delay-call for J1_w_del
. If it helps the discussion we could state that delay has time
as hidden argument.
- It is just confusing and not usable.
Basically I don't see anyone writing: parameter Real p1, p2=delay(p1, 1+time/2);
which is the case that we explicitly make an exception for. I don't see any benefit in making an explicit exception for something that will not be used by people making models, and I just expect it to confuse them - and make them wonder why we are spending time on this.
To me even allowing parameter Real p1, p2=delay(p1, 1);
is weird (see previous item), but as long as we don't give it as example it just follows from the normal rules, so I don't care.
If we change the rules to state that delay is never a parameter-expression that works as well, and would work even if we add add initialization for the delay.
In #3640 it was noted that both
delay
andspatialDistribution
should be event-generating, so that they can preserve discontinuities. This is particularly useful for dealing with discrete-valued signals, and opens up for also supporting non-Real
signals.We have now a test implementation of event-generating
delay
, and I'd say it works great. At the moment, describingdelay
accordingly is the only thing this PR does. For the much less frequently usedspatialDistribution
we don't have a test implementation at the moment, but would be ready to also go ahead and change the specification based on test implementations in other tools.Some questions to think about when making this change:
delay(1.0, delayTime, delayMax)
a constant expression.spatialDistribution
if we were to add event generating semantics?