Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#5301 closed defect (fixed)

The NF does not evaluate final parameters computed by functions of parameters with Evaluate = true and literal constants

Reported by: Francesco Casella Owned by: Per Östlund
Priority: high Milestone: 2.0.0
Component: New Instantiation Version:
Keywords: Cc: Karim Adbdelhak

Description

Some MultiBody models still do not produce the correct results (or possibly fail) when the NF is employed to flatten the model. For example, the results of Modelica.Mechanics.MultiBody.Examples.Elementary.HeatLosses do not match with the reference, in particular the z-axis component of the angular velocity w_a[3] is initialized to zero instead of 2 deg/s, and thus remains constant instead of oscillating.

Some analysis reveals that the start attribute of w_a[3] is computed using R_start.T, which is incorrectly computed to zeros[3,3] instead of diagonal(1, 1, 1).

The NF-flattened model sets the fixed attribute of R_start.T to false, and adds the initial equation

  body1.R_start = Modelica.Mechanics.MultiBody.Frames.axesRotations({1, 2, 3}, body1.angles_start, {0.0, 0.0, 0.0});

However, the Body model contains the following code

  final parameter Frames.Orientation R_start=
      Modelica.Mechanics.MultiBody.Frames.axesRotations(
        sequence_start,
        angles_start,
        zeros(3))
    "Orientation object from world frame to frame_a at initial time";

Since R_start is final and both sequence_start and angles_start have Evaluate = true, R_start should be marked as structural and evaluated by the front-end.

The issue is similar to #5084; in that case the parameter had Evaluate = true, while here it is final, but the end result should be the same.

I suspect this issue is probably the cause of other simulation failures or errors with the MultiBody library, which are due to the lack of constant evaluation of such rotation matrices, preventing the proper symbolic manipulation of the multibody system equations. I would suggest to fix this asap to improve the coverage and avoid getting wrong results from models that simulate successfully.

Change History (6)

comment:1 by Per Östlund, 6 years ago

The issue seems to be that angles_start actually does not have an Evaluate-annotation, it's just a normal non-final parameter. Adding Evaluate = true to the definition of angles_start causes it and the binding of R_start to be evaluated as expected.

Right now we only evaluate bindings of complex components if the component or the binding is structural. One solution would be to also try to evaluate bindings of non-structural complex parameters, and only move the binding to an equation if the evaluation fails. We could also restrict it to only final parameters if that's more appropriate.

in reply to:  1 ; comment:2 by Francesco Casella, 6 years ago

Replying to perost:

The issue seems to be that angles_start actually does not have an Evaluate-annotation

You're right, I was mistaken, Evaluate = true is actually on angle_fixed.

Adding Evaluate = true to the definition of angles_start causes it and the binding of R_start to be evaluated as expected.

Hmm, another place where we should patch the MSL. I'd rather avoid that.

Right now we only evaluate bindings of complex components if the component or the binding is structural. One solution would be to also try to evaluate bindings of non-structural complex parameters, and only move the binding to an equation if the evaluation fails. We could also restrict it to only final parameters if that's more appropriate.

I would not evaluate all non-structural complex parameters: a reasonable use-case is to use records to store component data, and then change them on the fly when re-simulating.

I think that evaluating final parameters (ad moving them to initial equations if the evaluation fails) would be an excellent compromise.

In principle, the fact that a parameter has a final modifier doesn't necessarily mean it should be constant evaluated, because the bound expression may not be structural (as in this case). But I guess that's what most people writing libraries expect, and that's also WWDD. So I would go for that.

in reply to:  2 comment:3 by Per Östlund, 6 years ago

Replying to casella:

I think that evaluating final parameters (ad moving them to initial equations if the evaluation fails) would be an excellent compromise.

Ok, I've implemented that in e11c6c60 (I had already implemented it, was just waiting for you to comment :)). We'll see how it affects the libraries.

comment:4 by Per Östlund, 6 years ago

Resolution: fixed
Status: newclosed

It seems like evaluating final complex parameters was mostly positive, see results. It fixed simulation of a couple MultiBody and Spice3 models, but also broke simulation of one Spice3 model.

It also didn't fix the HeatLosses model mentioned in the description but instead caused it to simulate much slower than expected, so it seems there's still some issue with that model. But I'll close this ticket since this particular issue is fixed, and you can open a new ticket if you figure out what the remaining issue is.

in reply to:  4 comment:5 by Francesco Casella, 6 years ago

Replying to perost:

It seems like evaluating final complex parameters was mostly positive, see results. It fixed simulation of a couple MultiBody and Spice3 models,

That's quite an understatement. I see 8 more MultiBody models in ModelicaTest now passing verification (which likely means their previous results were plain wrong), and also 4 more Spice3 models going through verification. Overall +1.3% of global coverage. Not bad for one commit :)

but also broke simulation of one Spice3 model.

I'll investigate that later.

It also didn't fix the HeatLosses model mentioned in the description but instead caused it to simulate much slower than expected

I looked at the trajectories reported by the verification tool, and they are oddily irregular. I guess there is still some structural issue that leads to a poorly conditioned state choice. I'll investigate that as well.

comment:6 by Francesco Casella, 6 years ago

Cc: Karim Adbdelhak added

I need to reconsider my position on this topic. I ran the HeatLosses model on Dymola, and I noticed that I can actually change body1.angles_start at runtime, and that doing so indeed influences body1.R_start.T accordingly.

So, my statement in comment:2 was indeed wrong: Dymola does not evaluate final parameters if they depend on non-final, non-structural parameters without an Evaluate annotation, as we currently do with both the old and new frontend, after commit e1c6c60.

I guess it is ok for the time being if we do so in this specific case, in which records contining arrays are involved, and we don't want those bindings to end up in the initial equation section. However, I guess we should eventually avoid evaluating final parameters unless they *exclusively* depend on structural or final parameters, and the back-end should handle that case correctly.

I just opened #5390 on this topic so we can track it.

Note: See TracTickets for help on using tickets.