Opened 6 years ago
Closed 6 years ago
#5287 closed defect (fixed)
Evaluation of propagated parameters with Evaluate = true
Reported by: | Francesco Casella | Owned by: | Per Östlund |
---|---|---|---|
Priority: | high | Milestone: | 2.0.0 |
Component: | New Instantiation | Version: | |
Keywords: | Cc: | Lennart Ochel |
Description
Please consider the following test case (that I suggest to add to the testsuite)
package P model M parameter Real tau; Real x; equation tau*der(x) + x = sin(time); end M; model S parameter Real tau = 0 annotation(Evaluate = true); M m(tau = tau); end S; model S2 parameter Real tau = 0 annotation(Evaluate = true); M m(final tau = tau); end S2; end P;
The old front-end has a greedy tendency to evaluate stuff, so it evaluates both tau
and m.tau
in both P.S
and P.S2
, allowing the back-end to eliminate the product tau*der(x)
and avoid the structural singularity.
The NF instead does not evaluate m.tau
in either case, leading to a numerically singular model that cannot be solved.
In the case of P.S2
, there is no question that the parameter m.tau
should be evaluated to zero by the front-end, because the final modifier makes it identically equal to tau
, which has the Evaluate=true
annotation.
The case of P.S
is a bit more controversial. In principle, since the modifier is not final, one may want to change the binding of m.tau
to some other value at runtime, so the front-end may not evaluate it; however, this means that the tau = 0
case would end up in a singular simulation model.
This issue affects for sure Modelica.Electrical.Analog.Examples.HeatingNPN_OrGate and Modelica.Electrical.Analog.Examples.HeatingPNP_NORGate. I opened PR #2812 on the MSL issue tracker, you can follow the discussion there.
No matter how that discussion will end, I suspect this pattern shows up in other places in the MSL and in other libraries, and causes lots of weird run-time errors on models that otherwise run fine if built with the OF.
I reckon that in all cases of use of the pattern shown in P.S
, the intention of the modeller is that the propagated parameter m.tau
also gets evaluated, even in the absence of a final
qualifier. From an end-user perspective, expecting that the Evaluate = true
property is somehow propagated feels quite natural, and, incidentally, that's WWDD.
So, I would also propose to add a heuristic rule that parameters who are bound to a parameter with the Evaluate = true
annotation are marked as structural and evaluated. I guess this would solve a number of runtime issue and increase the coverage of the NF significantly. We may later on relax this rule if there are use cases which are negatively affected, but I can't think of any at this time.
Change History (8)
comment:1 by , 6 years ago
comment:2 by , 6 years ago
Yeah, please do so if it's not too complicated. I'm afraid the discussion on PR 2812 will be long, I'm not sure of the outcome, and i don't really want to spend time fixing third party libraries in this respect, since they are not really breaking the specification.
follow-up: 4 comment:3 by , 6 years ago
In 28f7795 the NF now mark parameters that have a structural cref as binding as also structural.
Note that this only applies when the binding consists of a single cref and nothing else, i.e. parameter Type c1 = c2;
will cause c1 to be marked as structural if c2 is structural. Doing the same for more complex bindings causes issues that are not trivial to fix, so for now we restrict it to only crefs.
But I think that should cover the majority of the cases anyway, at least it fixes the models mentioned in the ticket and also PNlib that i mentioned. We'll see what the results of the full library testing are.
follow-up: 5 comment:4 by , 6 years ago
Replying to perost:
Note that this only applies when the binding consists of a single cref and nothing else, i.e.
parameter Type c1 = c2;
will cause c1 to be marked as structural if c2 is structural.
Absolutely! That was my intention.
Doing the same for more complex bindings causes issues that are not trivial to fix, so for now we restrict it to only crefs.
I'd definitely avoid going down this rabbit hole. In this case, though, at least the case with final should work, e.g.:
model S3 parameter Real tau = 0 annotation(Evaluate = true); M m(final tau = 2*tau); end S3;
But I think that should cover the majority of the cases anyway, at least it fixes the models mentioned in the ticket and also PNlib that i mentioned. We'll see what the results of the full library testing are.
Agreed.
comment:5 by , 6 years ago
Replying to casella:
I'd definitely avoid going down this rabbit hole. In this case, though, at least the case with final should work, e.g.:
model S3 parameter Real tau = 0 annotation(Evaluate = true); M m(final tau = 2*tau); end S3;
That won't work with the changes I made, but I guess we could change the restriction from a single cref to an expression involving crefs and literals. Or an expression not involving function calls perhaps. There just needs to be some clearly defined pattern we can check against.
comment:7 by , 6 years ago
Cc: | added |
---|
I saw the outcome of your commit 8a20ce05c, which is indeed quite good: the problem reported above with the Modelica.Electrical
models is solved, and it seems this also had a very positive outcome on PNLib
and other libraries.
I will close the ticket for the time being, since I don't think what I was asking for in comment:5 is actually needed anywhere, now that we are propagating Evaluate on single cref bindings.
comment:8 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
This also affects a lot of models in PNlib, where some equations involving empty arrays are kept even though the variables involved are not part of the DAE. The Evaluate annotation is not used in those models either, only structural non-final parameters.
I experimented with propagating "structuralness" via modifiers (making parameters with structural bindings structural), but it was causing some other issues and I eventually moved on to more important issues. But if there are models in the MSL affected by this too I guess I should take a second look.