Opened 5 years ago

Closed 5 years ago

#5287 closed defect (fixed)

Evaluation of propagated parameters with Evaluate = true

Reported by: casella Owned by: perost
Priority: high Milestone: 2.0.0
Component: New Instantiation Version:
Keywords: Cc: lochel

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 Changed 5 years ago by perost

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.

comment:2 Changed 5 years ago by casella

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.

comment:3 follow-up: Changed 5 years ago by perost

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.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 5 years ago by casella

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 in reply to: ↑ 4 Changed 5 years ago by perost

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:6 Changed 5 years ago by casella

What about marking as structural all parameters with a final modifier?

comment:7 Changed 5 years ago by casella

  • Cc lochel 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 Changed 5 years ago by casella

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.