Models with experiment annotation that have parameters without bindings and without explicitly set start values in ThermoFluidStream

[casella]

Dear ThermoFluidStream developers,

please check the latest OpenModelica regression test report. There are a few ThermoFluidStream models that fail since we fixed ticket OpenModelica/OpenModelica#10386.

According to the latest version of Modelica Specification, i.e., 3.6, when you simulate a model all parameters with fixed = true should have a binding equation (either provided as a default in the components, or set by modifiers when instantiating them). The only exception is parameters that have an explicitly set start attribute - in that case, if a binding equation is lacking, the start attribute is used to set the parameter value and a warning should be issued.

You can

• add a default binding to the parameters in the component definitions, or
• add an explicit bining when you instantiate the components or,
• add an explicit start = 0 attribute to the parameters in the component definition, which will be used as a fallback value, with a warning being issued.

This will make those models compliant with the Modelica Specification in force, and get them running again in OpenModelica.

Thanks!

[tobolar]

@casella Thanks, we will fix this. I only wonder why this pops up when using Modelica 4.0.0 in your tests, which in fact relies on Modelica Specification version 3.4.

[casella]

@tobolar, the documentation of MSL 4.0.0 states that it is based on the language version 3.4, but I'm not aware of annotation that explicitly gives this information to the compiler. Do I miss something?

That said, this specific point is an improvement that will improve the quality of the library, regardless of the version of the library, so I would do it anyway.

Thanks!

[tobolar]

the documentation of MSL 4.0.0 states that it is based on the language version 3.4, but I'm not aware of annotation that explicitly gives this information to the compiler.

Neither am I.

That said, this specific point is an improvement that will improve the quality of the library, regardless of the version of the library, so I would do it anyway.

Yes, definitely!

[casella]

PR #187 fixes the issue.

BTW, in any case, according to the specification, you always get warnings about parameters without a default binding. This not nice and potentially quite confusing for end-users, since in fact they can't do anything about them, as the corresponding parameter editing fields are actually disabled.

My suggestion is to add an explicit default binding to zero, and then add some asserts to make sure that the user has not forgotten to set the relevant parameters to some non-zero value, e.g.:

assert(Cvs_US > 0 or not flowCoefficient == FlowCoeffType.Cvs_US, "Please set Cvs_US to some non-zero value");

In this way, you get safe behaviour, no warnings, and full MLS compatibility. I can open a PR with this change, if you want.

My 2 cts 😃

[nieweber]

Thanks a lot for your remarks, Francesco! :)

We already had a lot of discussions about this topic internally as we didn't find a solution that we liked. The warnings about the parameters without default binding indeed is not really nice as it might also confuse the user. We also considered the solution you suggested with using asserts to inform the user about invalid input. From our perspective the check of the valid inputs should be done by the tool and the user should be informed when checking/translating the model.

@tobolar propsed a solution to make the start values dependent from each other what do you think about this? Do you see any major disadvantages in this suggestion? :)

[dzimmer]

I have no reservations against the solution provided by @tobolar. Actually, I really like it. :-) @tobolar: Please proceed with a pull request to close this issue.

[casella]

We also considered the solution you suggested with using asserts to inform the user about invalid input. From our perspective the check of the valid inputs should be done by the tool and the user should be informed when checking/translating the model.

If you add annotation(Evaluate = true) to Cvs_US and flowCoefficient, the assert would be evaluated at compile time/check time. On the other hand, you lose the possibility to tune the valve coefficient at run time, which could be useful e.g. for system sizing application, so it's not a perfect solution either.

@tobolar propsed a solution to make the start values dependent from each other what do you think about this? Do you see any major disadvantages in this suggestion? :)

@tobolar proposed the following code:

  parameter Real Kvs(unit = "m3/h", start=Cvs_US/1.1561)  "Kvs-value (metric) from data sheet (valve fully open)";
  parameter Real Cvs_US(start=1.1561*Kvs) "Cvs-value (US [gal/min]) from data sheet (valve fully open)";

I remember we discussed this pattern during some Modelica Design Meeting many years ago. The problem with this pattern is that if you don't set a modifier for any of these two parameters, according to Section 8.6 the compiler adds the binding equations with the start attributes, and then you get binding equations with cyclic dependency:

- Kvs = Cvs_US/1.1561
- Cvs_US = 1.1561*Kvs

which is not allowed by Section 4.4.3 😅

In practice there are two possible outcomes in this case:

1. if the tool is strict in enforcing the specification, you get a "cyclic parameter binding is not allowed" error, which is quite confusing for an user that should rather be warned to set at least one valve size parameter;
2. if the tool is more lax (i.e., it doesn't do the BLT on the start attributes), you get arbitrary results.

If you try this MWE:

model M
  parameter Real p(start = q);
  parameter Real q(start = p);
end M;

OMC fails at compile time with

Translation Error
Cyclically dependent parameters found:
         q:PARAM(start = p )  = p M type: Real
         p:PARAM(start = q )  = q M type: Real

while Dymola 2024 x also fails at compile time with

The parameter bindings 
q = p;
p = q;
are cyclic, that is only allowed if evaluating other parameters with Evaluate=true breaks the loop.

[dzimmer]

Ok...Now I understand the issue. The core problem is the abuse of Evaluate=True to suppress the error in case of incomplete parameterization. This is (of course) not the purpose of Evaluate=True and the curse of mixing concepts. Evaluate=True has to be removed. This is simply an abuse of the concept.

The latest suggestion of Francesco with the assert is the only reasonable option I see.

[casella]

At least that is safe, because the assert prevents the user to run an incorrectly parameterized model. That is mandatory IMO. Too bad the feedback comes at runtime, but I guess given the current status of the language, we'll have to live with that.

[nieweber]

Thanks for your thorough review, for me the solution with the assert is totally fine then! :)

[tobolar]

propsed a solution to make the start values dependent from each other what do you think about this? Do you see any major disadvantages in this suggestion? :)

I delete that branch since there is the PR #201 resolving this issue.