Changed default of PadeDelay to robust implementation (balance = true)

[casella]

A few days ago @GiovanniMangola brought my attention on the Modelica.Blocks.Nonlinear.PadeDelay model.

If balance = true, the model works just fine: the implementation is numerically sound, and so it is the initialization, which is full steady-state and corresponds exactly to what would happen if you had a pure delay model, which is by definition initialized in steady-state, see the Specification.

However, for reasons of backwards compatibility with MSL 3.2.1, the default is still balanced = false, which provides a numerically ill-conditiond textbook implementation, and furthermore has only one initial equation instead of N, leading to a model with missing initial equations, which is in general not a good idea.

In the case of our application, the block (with the default choice) was put inside a big plant model, which failed miserably to initialize because some other (unfortunately unwanted and wrong) initial equations were picked instead by Dymola, because of the incompletely specified initial problem stemming from the lack of initial equations in the block. I think this a very dangerous behaviour that should be avoided. What initial equations are needed is very clear for this model, there is really no point at leaving them out for reasons of backwards compatibility.

I think the transition to MSL 4.0.0 is a good opportunity to get a better behaviour, even though it is slightly non-backwards compatible. This means:

• making balance = true the default (there's no point having a numerically ill-conditioned implementation as default if we have a sound one available)
• adding full initial equations for the case balance = false, so one does not have to rely on the tool to pick them, potentially in an incorrect way, as it happened to us, in case you really want to use that implementation for some reason

I guess this may be listed in the release notes.

I also updated the two tests in ModelicaTest accordingly. Both compare the output and performance of the block with the old one from MSL 3.2.1, which is copied inside the test cases. I slightly changed that copy to always have full initial equations. We agreed long time ago that all the examples in MSL and ModelicaTest should have fully specified initial conditions to avoid ambiguities in the selection of initial equations with different tools, so this change is necessary to fulfill that requirement.

[beutlich]

This looks like a reasonable change indeed, but did you actually forget to change the default value of balance from false to true?

Changing default values requires conversion rules and test model(s), whhich are missing. Since we won't touch upcoming MSL 4.0.0 to introduce any new conversion rules, we need to postpone this PR until MSL 4.0.0 is released and branched to maint/4.0.x.

[beutlich]

see my above comment

[MartinOtter]

This pull request is a good idea, but please improve:

• balance=true is not set as default (should be changed).
• I would keep the balance=false exactly as it is (so no change of initial conditions), in order that exisiting models are not influenced by this change. With a conversion script it should be (somehow) defined that models without balance definition, use balance=false (= the current definition)

[casella]

This pull request is a good idea, but please improve:

* balance=true is not set as default (should be changed).

Sorry about that, I don't know why this happened. Just added a new commit to the PR.

* I would keep the balance=false exactly as it is (so no change of initial conditions), in order that exisiting models are not influenced by this change. With a conversion script it should be (somehow) defined that models without balance definition, use balance=false (= the current definition)

@MartinOtter, I'm not sure I get your point here. I see the lack of initial equations as a bug, because it forces to rely on the tools' automatic addition of initial equations, which is dangerous and may lead to wrong results.

I don't think users ever added those extra initial equations themselves manually when using balance = false, they were just relying on the tool to add them automatically. Adding them explicitly should just reduce the chance of getting wrong initial conditions.

Furthermore, we agreed long time ago that all examples in MSL and ModelicaTest have fully specified initial conditions. If I remove those initial equations, should I add extra initial equations to the two ModelicaTest test models? That looks a bit ugly to me.

[casella]

With a conversion script it should be (somehow) defined that models without balance definition, use balance=false (= the current definition)

I read the specification of convertModifiers carefully, but I really have a hard time figuring out exactly how to do that. See the discussion in #2518.

[beutlich]

Here you go:

convertModifiers({"Modelica.Blocks.Nonlinear.PadeDelay"}, fill("",0), {"balance=false"})

[MartinOtter]

@MartinOtter, I'm not sure I get your point here. I see the lack of initial equations as a bug, because it forces to rely on the tools' automatic addition of initial equations, which is dangerous and may lead to wrong results.

I don't think users ever added those extra initial equations themselves manually when using balance = false, they were just relying on the tool to add them automatically. Adding them explicitly should just reduce the chance of getting wrong initial conditions.

Furthermore, we agreed long time ago that all examples in MSL and ModelicaTest have fully specified initial conditions. If I remove those initial equations, should I add extra initial equations to the two ModelicaTest test models? That looks a bit ugly to me.

The problem is that users might have added enough initial conditions in their models and with
a new MSL version, suddenly they will get an error, even if their models are correct.
The version with balance=false should anyway never be used (I see no use case for it).
It is then more important that users that utilized this model already (with balance=false)
do not get a different behaviour (in the worst case with "errors") .

[casella]

The problem is that users might have added enough initial conditions in their models and with
a new MSL version, suddenly they will get an error, even if their models are correct.
The version with balance=false should anyway never be used (I see no use case for it).
It is then more important that users that utilized this model already (with balance=false)
do not get a different behaviour (in the worst case with "errors") .

Should I then add the extra initial equations manually to the ModelicaTest?

[casella]

Here you go:

convertModifiers({"Modelica.Blocks.Nonlinear.PadeDelay"}, fill("",0), {"balance=false"})

@beutlich, are we sure?

The specification says

If OldModifer list is empty it (= the modifier) is added for all uses of the class

In this case fill("",0) is an empty list, so I understand balance = false will be added for all uses of the class. But that's not what I want, I only want that to happen when there is no modifier in the model to be converted. If there is a balance = true modifier already, nothing at all should be added or changed.

Do I miss something?

Also (minor comment), why are we using an array of strings with one element as the first argument and not just the string, as in

convertModifiers("Modelica.Blocks.Nonlinear.PadeDelay", fill("",0), {"balance=false"})

[dietmarw]

fill("",0) will only hit if there is no balance defined.

@casella I don't understand your second question as you are just restating the exact same conversion rule. Did you mean that you wonder why the syntax is not:

- convertModifiers("Modelica.Blocks.Nonlinear.PadeDelay", fill("",0), {"balance=false"})
+ convertModifiers("Modelica.Blocks.Nonlinear.PadeDelay", "", {"balance=false"})

then I agree but maybe @HansOlsson can explain why it needs to be an empty array.

[HansOlsson]

@casella I don't understand your second question as you are just restating the exact same conversion rule. Did you mean that you wonder why the syntax is not:

- convertModifiers("Modelica.Blocks.Nonlinear.PadeDelay", fill("",0), {"balance=false"})
+ convertModifiers("Modelica.Blocks.Nonlinear.PadeDelay", "", {"balance=false"})

then I agree but maybe @HansOlsson can explain why it needs to be an empty array.

Well, if viewed as a function it is a function with a String-array as argument (in order to be able to support multiple old modifiers), and thus the current variant is straightforward.

Considered variants:

• Since the old-modifier-list and the new-modifier-list must be handled together and don't necessarily have the same length we cannot view it as a vectorized call (it would also have made it complicated to combine with vectorization of the class-name).
• We could have allowed an empty string as well with the same meaning, i.e. {""} - but it would have required a bit more text and there would have been more special cases to consider (both in the specification and in implementations).
• There is also some old idea that the empty array {} would just magically be of the right type - and thus that could be used here, but that is currently not part of the specification.

[HansOlsson]

This pull request is a good idea, but please improve:

• balance=true is not set as default (should be changed).
• I would keep the balance=false exactly as it is (so no change of initial conditions), in order that exisiting models are not influenced by this change. With a conversion script it should be (somehow) defined that models without balance definition, use balance=false (= the current definition)

An alternative would be to make the initial equations conditional on a new flag.

That way users don't have to manually add the initial equations, but it doesn't break models that have added initial equations. (I would in general say that it should be possible to disable initial equations.)

I haven't investigated this to see what the defaults for this flag should be (ideally they should be compatible with existing models - but if they differ depending on balance, and the default for balance is changed then it becomes a bit complicated).

[beutlich]

Conversion script still is missing.

[casella]

Conversion script still is missing.

Sure. That will take some time. After the coronavirus hit I no longer had time to deal with that. Will take care in September, when we'll implement them in OMC.