Missing warning for StateSelect.always

[sjoelund.se]

The following model does not generate warnings, but should in fact produce errors:

model M
  Real level(start=0, fixed=true, stateSelect=StateSelect.always) = r;
  Real r=2.0;
end M;

[sjoelund.se]

​https://github.com/OpenModelica/OMCompiler/pull/2191

[casella]

Rescheduled to 1.14.0 after 1.13.0 releasee

[casella]

@Karim, I guess this issue goes hand in hand with the state selection issues you are already working on, namely #5574 and #5590.

[Karim.Abdelhak]

Replying to Karim.Abdelhak:

I wanted to change another ticket, just a mistake!

[casella]

Releasing 1.14.0 which is stable and has many improvements w.r.t. 1.13.2.

This issue, previously marked as blocker for 1.14.0, is rescheduled to 1.15.0

[casella]

Release 1.15.0 was scrapped, because replaceable support eventually turned out to be more easily implemented in 1.16.0. Hence, all 1.15.0 tickets are rescheduled to 1.16.0

[Karim.Abdelhak]

Index reduction just decides to make it a dummy state, because there is no choice. Should it always fail on trying to convert a state with StateSelect.always to a dummy state? This could break some models i assume...

[Karim.Abdelhak]

Presumably fixed in ​PR6669.

Contrary to what i thought, it was removeSimpleEquations which tried to convert the state to a dummy state. It now is not allowed to do that anymore if the variable has StateSelect.always and throws an error:

Error: Variable level has attribute stateSelect=StateSelect.always, but can't be selected as a state.

Afterwards it is throwing some follow up errors from index reduction, which is fine i think:

Error: Internal error It is not possible to select continuous time states because Number of Equations 2 greater than number of States 0 to select from.
Error: Internal error - IndexReduction.dynamicStateSelectionWork failed!

I will close this ticket if the tests run fine.

[casella]

Replying to Karim.Abdelhak:

Should it always fail on trying to convert a state with StateSelect.always to a dummy state?

Yes. The specification of StateSelect.alwaysis very clear:

• ​Section 4.8.8.1 "Do use it as a state"
• ​Section 3.7.3, explanation of reinit(): "x is a Real variable (or an array of Real variables) that is implicitly defined to have StateSelect.always [so must be selected as a state, and it is an error, if this is not possible]

This could break some models I assume...

So be it. Our policy is to stick to the specification. The alternative is to have illegal Modelica code hanging around for decades (see the 'each' saga in the MSL and other libraries).

[Karim.Abdelhak]

The PR is merged and it works as expected. I also updated the compliance checks because they now fail as expected for StateSelect.always variables if they cannot be selected as states.

[casella]

Wunderbar :)

[Karim.Abdelhak]

It rejects some Models from ThermalSeparation, but as far as i can see rightfully so. I suppose someone with knowledge about these models should check whether or not the StateSelect.always attributes are sensible.

• ​regression report

[casella]

See ​ThermalSeparation PR #3

[casella]

After consulting Hans Olsson, it is probably better to make this a warning, rather than an error. I see no reason to be overly restrictive here, particularly if Dymola thinks otherwise

[Karim.Abdelhak]

I re-visited my commit and noticed that it does not hard quit on some things. If there is a direct undeniable connection between two states and they both have StateSelect.always (it gets removed by removeSimpleEquations) than it fails and reports the error. If StateSelection makes a choice which leads to the removal of a StateSelect.always variable on the other hand, it reports an error but does not fail. I think this is an okay behavior since the latter is a heuristic in some regards.

I could still revoke the other part to also be a warning and not fail.

What do you think?

[casella]

Honestly I don't see much of a difference. In both cases you must do index reduction (one way or the other) and in principle you should be barred from doing that because you really, really want to have those variables as states.

At this point I'd go for the warning in both cases for consistency. Just make the warning message serious enough, like:

Variable XYZ has stateSelect = StateSelect.always but cannot be selected as state. It is recommended to downgrade stateSelect to StateSelect.prefer to reflect what it is actually possible to do with this model.

[Karim.Abdelhak]

I reverted it to a warning in ​PR6680.

Unfortunately that lead to the compliance test failing again, but that was to be expected.

[casella]

Replying to Karim.Abdelhak:

I reverted it to a warning in ​PR6680.

Shall I merge it in as it is? Looks good to me.

Unfortunately that lead to the compliance test failing again, but that was to be expected.

I checked ​https://test.openmodelica.org/compliance/compliance-current.html but I'm not sre what are you referring to. AttributeStateSelectInvalidAlways passes, as expected, right?

[Karim.Abdelhak]

As you can see in ​my changes i added that test to the failing ones (failing in the sense of compliance).

There is a lot of double negation going on here but the basic thing is: The modelica language specification states that the model should fail. So if the model fails it succeeds for our compliance check. Since it does not fail we do not comply to that (with my old changes it did fail).

You can push it, seems like people are working on other stuff right now and cannot push it.

[casella]

Replying to Karim.Abdelhak:

As you can see in ​my changes i added that test to the failing ones (failing in the sense of compliance).

There is a lot of double negation going on here

I don't think this is not the case :)

but the basic thing is: The modelica language specification states that the model should fail.

I though this was not explicitly mentioned, so that's why I asked Hans. I looked up the test case in ​Modelica-Compliance

model AttributeStateSelectInvalidAlways
  extends Icons.TestCase;

  Real v(stateSelect = StateSelect.always) = if time<1e-5 then 1.0 else 5.0;

  annotation (
    __ModelicaAssociation(TestCase(shouldPass = false, section = {"4.8.7.1"})),
    experiment(StopTime = 0.01),
    Documentation(
      info = "<html>Checks that StateSelect.always is not misused. 
       Note that if we had v=5.0 it could be differentiated and v selected as a state (with derivative zero).</html>"));
end AttributeStateSelectInvalidAlways;

This interpretation is interesting, basically it seems that in case you had

model foo
  Real x(stateSelect=StateSelect.always);
equation
  x = sin(time);
end foo;

this should basically be legal and be translated into

model foobar
  Real x;
  Real der_x;
equation
  der_x = cos(time); // dummy der
  der(x) = der_x; // actual state equation
initial equation
  x = sin(time);
end foobar;

Of course the question would be "why on Earth one should want this", but that's exactly what the specification says: "Do use x as a state".

Of course you can't do that if the expression cannot be differentiated, hence the compliance test should fail, not pass with a warning.

I'm keeping this ticket open so we can continue the discussion this fall, possibly involving Hans or the entire MAP-LANG group. For the time being I guess it is better to adopt a tolerant approach rather than a strict one, so I'm fine with the PR having been pushed.

You can push it, seems like people are working on other stuff right now and cannot push it.

Adrian did it in the meantime.

[Karim.Abdelhak]

Well i understand what you mean, but i am not entirely sure how to detect that specific case.

Just to summarize, i would differ between two different kinds of "StateSelect.always not possible":

1. Remove simple equations detects equated states with both StateSelect.always. One has to be removed.
2. Index Reduction cannot select a state with StateSelect.always due to structural restrictions such as the one you mentioned (the equation cannot be differentiated). But i think there might be more reasons for this.

If i understood you correctly 1. should not fail anymore since dymola does not fail on it. And 2. should only fail for this condition (?)...

[casella]

Replying to Karim.Abdelhak:

If i understood you correctly 1. should not fail anymore since dymola does not fail on it. And 2. should only fail for this condition (?)...

Possibly. But I'm not sure.

I would suggest to prepare some MWEs (which should eventually get into the Modelica Compliance library) that single out all these cases involving StateSelect.always, then open an issue on the Modelica Specification issue tracker, start the discussion, and try to get some consensus also involving other tool vendors. I can help with that in September, now I'm running out of time before the holidays. If you want to go ahead with it right now, I can act as internal reviewer before we post the issue.

[casella]

This is no longer a blocker issue, it involves clarifications with the MAP-LANG group

[casella]

Rescheduled to 1.18.0

[casella]

Ticket retargeted after milestone closed