-
Notifications
You must be signed in to change notification settings - Fork 43
Add variability change for these functions. #3610
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
So, an Integer output will be discrete-time, but a Real output still non-discrete time (assuming the inputs are non-discrete time).
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 only have a chance of working if GenerateEvents = true
was applied recursively also to all function calls in the function body. We need to avoid loopholes like this:
model DiscreteValuedFunctions
function f
input Real u;
output Integer y = integer(u + 0.5); /* Non-discrete-time Integer output. */
annotation(Inline = false); // Avoid SystemModeler issue.
end f;
function g
input Real u;
output Integer y = if u < 2 then 0 else f(u);
annotation(GenerateEvents = true);
end g;
Real a = g(time); // Fine, result with discontinuity at time 2.0
//Integer b = g(time); // Bad: b is assigned a non-discrete-time solution
annotation(experiment(StopTime = 5.0, Interval = 0.1));
end DiscreteValuedFunctions;
Edit: Improved example.
A good point, and it's possible that such a recursive solution would work, but I would rather propose to rethink this in another way that gives a similar result in this case. A function with The problem with applying it recursively is that it must be done with care.
Naively generating events would imply that
Totally agree. |
Co-authored-by: Henrik Tidefelt <[email protected]>
I have now added some text for that. We could add an example showing how this requires that some called functions also use the annotation. |
I think we'll have to rethink the variabilities of function inputs when It would be great if setting
It would seem unfortunate if the following would be a variability error inside the function:
So the question is whether it would be possible to utilize the expression variability of arguments passed to the function, instead of only using the worst-case variability for the type of the input. |
I actually believe there is a natural and elegant solution to the problem with
That is, declared variability of function inputs. It would primarily be useful for functions with Some details:
|
I see the point, and agree to an extent - and will return to that. However, it doesn't need discrete-time variability for I can agree that it is inefficient to generate an event for a literal (or parameter) input, but I don't see that it is an error. It is also a bit confusing that the outputs from functions have two variabilities:
But that isn't a new problem, and I don't see that it generates problems. In practice the usual implementation of GenerateEvents=true is to simply inline the function (in contrast to the complicated inlining for other functions that need to handle events), and after that the expression variability of the arguments will be used to avoid generating events. |
Sorry, my example was bad. What I meant was rather something like this:
Again, the elegant solution to this would be the following:
|
To me the function
I have a bit of problem seeing the use-case for this; having a function generating events and then declaring that the inputs are discrete mean that function will not generate events. And remember that the simple implementation of GenerateEvents is to inline it, so the case of a function having multiple real inputs - some discrete and some non-discrete - seems quite esoteric. |
The point would be to have rules for variability checking that allow for separate checking of the function body and the expressions where the function is called. This is something we (System Modeler) rely heavily upon when Separate variability analysis of the function body also makes the function a much better abstraction, and allows errors to be reported in a simpler way than when the error is a joint effect of the body of the function and the arguments passed. |
I agree that such a separation is a good thing, but I don't see why it is necessary to declare some inputs as discrete for that. |
…omplicated, with link.
In the example above,
It is thanks to If the |
But that requires a new deduction rule for discrete variables and expressions. That will in itself require an MCP, so it seems best to first add this PR without it - since this PR fixes the current inconsistency.
With the proposed rule the function f (without discrete) would fail, since g doesn't generate events. To later extend the rules to handle some calls of such a function - seems like a later problem. |
Poll for adding GenerateEvents-variability so that the following works:
With GenerateEvents=true: (normally implemented as "simple inlining"):
Favor: Hans, Francesco, Elena, Gerd, Markus (but unsure about specifics) |
This has now been test-implemented in Dymola, without any issues (so the ones that should work do work, and the problematic ones give reasonable diagnostics). |
@HansOlsson to add examples to pull-request and check rules for inputs to GenerateEvents-functions, and phase and state in Media-library. |
I have added examples and checked that those inputs behave as normal variables, so Integer, Boolean, String are discrete-time (which works for things like phase), whereas Real is non-discrete-time. The last two functions is sort of simplified variant of |
@henrikt-ma to check |
Ping @henrikt-ma |
Sorry, this is not a trivial thing to review, so I haven't found the time to do it yet. The good news is that the variability pro @qlambert-pro is back from parental leave, so I have asked him for help. |
@qlambert-pro and I had a look and would like to add two small models for discussion. I'll begin with the more concerning one.
Unless I misread the proposal, this would be accepted:
To fix this, we propose using declared variabilities of the function components:
Now, the variability issue is detected in the expression Note that |
Our second (and less severe) example is the following:
Our interpretation of the proposed design is that this is rejected due to Again, declared variability of function inputs and outputs would come to the rescue:
One might consider making When Without the |
I had to re-check, but that was not the intent.
To me there are two ways of changing this:
So basically change: The implication is that only the variability of the call is impacted (or non-impacted), but the validity is unchanged. In practice I could in MSL only find the phase-input as a non-Real input to such a function (for two-phase medium), and region for Water. As far as I understand they are clearly intended to be literal values so I don't see any problem with this restriction. (And I could not find any call where that phase/region was used; and a complication is the two main media deriving from two-phase medium are R134a and Water; both ignore the phase.) |
I don't see this as an issue, and to me the example just looks odd. Basically I don't see why anyone would use GenerateEvents=true, but then return a discrete-valued value as a Real in an actual model instead of just returning an Integer with those values. Having an integer output would also allow the obvious:
The reason for having different rules for Reals and non-Real outputs (and even considering real output from a function generating events) are instead cases such as:
(which I believe we cannot yet handle in Dymola). |
input Real x2; | ||
output Boolean b; | ||
algorithm | ||
b := x1 >= x2; |
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.
Excessive space:
b := x1 >= x2; | |
b := x1 >= x2; |
input Real x2; | ||
output Boolean b; | ||
algorithm | ||
b := x1 >= x2; |
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.
Excessive space:
b := x1 >= x2; | |
b := x1 >= x2; |
if x2 > 1 then | ||
y := 1; | ||
else | ||
y := x2*x2; |
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.
We have a nice operator for the square:
y := x2*x2; | |
y := x2^2; |
@@ -171,7 +171,7 @@ \section{Function as a Specialized Class}\label{function-as-a-specialized-class} | |||
\item | |||
For initialization of local variables of a function see \cref{initialization-and-binding-equations-of-components-in-functions}). | |||
\item | |||
Components of a function will inside the function behave as though they had discrete-time variability. | |||
Components of a function will inside the function with some exceptions behave as though they had discrete-time variability, see \cref{discrete-time-expressions} for details. |
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.
Below, it is said that inputs behave as if they were evaluable. Maybe we should just drop this item now that the rules are becoming more complicated?
\item | ||
Unless inside \lstinline!noEvent!: Function calls where the function has annotation \lstinline!GenerateEvents = true! (\Cref{modelica:GenerateEvents}), the output does not contain a subtype of \lstinline!Real!, and any non-\lstinline!Real! inputs have discrete-time variability. | ||
For a function call returning multiple return values (see \cref{output-formal-parameters-of-functions}) the variability is decided separately for each output. | ||
These calls will generate events, \Cref{modelica:GenerateEvents}. |
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 sentence is more of a distraction than helpful here:
These calls will generate events, \Cref{modelica:GenerateEvents}. |
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 can see now that we are homing in on something that would work.
We need to think about how to deal with the backward incompatibility, though. This function used to be valid, but should be rejected according to the new variability rules:
function f
input Real x;
output Boolean b = noEvent(x > 0.5);
annotation(GenerateEvents = true);
end f;
Of course, the above is just a minimal example. A slightly less minimal but more realistic example might include a call to another function without GenerateEvents = true
:
function g
input Real x;
output Boolean b = x > 0.5;
end g;
function h
input Real x;
output Boolean b = g(x);
annotation(GenerateEvents = true);
end h;
Based on discussion in modelica/ModelicaStandardLibrary#4495
I created it as a PR in order to have something concrete to discuss.
It is a draft PR since there are a number of issues to resolve (in addition to the obvious issues of an annotation having such an impact):
The reason it is a separate item is to make it easier to discuss.
I have slightly changed my mind on these items and will update the PR