modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
453 stars 165 forks source link

Generate events in functions; to avoid non-discrete booleans. #4263

Closed HansOlsson closed 2 weeks ago

HansOlsson commented 6 months ago

Closes #4209

I don't know if significant enough to push for release.

HansOlsson commented 6 months ago
model Model1b
  replaceable package Medium = Modelica.Media.R134a.R134a_ph "Medium model"
    annotation (choicesAllMatching=true);
  Medium.ThermodynamicState state_a;
  Modelica.Units.SI.AbsolutePressure p=Medium.p_default;
  Modelica.Units.SI.SpecificEnthalpy h=Medium.h_default;

equation 
  //state_a = Medium.setState_phX(Medium.p_default, Medium.h_default);
  state_a = Medium.setState_phX(p, h);
end Model1b;

still fails in Dymola 2024x - even with the proposed changes. Would be good to have this model as test model.

Are other functions affected as well?

No, but it seems that I messed up the annotations a bit, it should now be resolved.

HansOlsson commented 6 months ago

still fails in Dymola 2024x - even with the proposed changes. Would be good to have this model as test model.

I added a test-model, but made it similar to an existing one instead.

beutlich commented 6 months ago

I don't know if significant enough to push for release.

For me it seems like a MSL bug. What's the opinion of the Media officers?

beutlich commented 6 months ago

Unfortunately, the new test model ModelicaTest.Media.TestOnly.R134a_setState_phX raises a AssignmentFromNonDiscreteToDiscrete warning in SimulationX 4.4 and fails to simulate with all tested solvers due to a singular system of equations. It does not matter if this PR is applied or not.

HansOlsson commented 6 months ago

Unfortunately, the new test model ModelicaTest.Media.TestOnly.R134a_setState_phX raises a AssignmentFromNonDiscreteToDiscrete warning in SimulationX 4.4 and fails to simulate with all tested solvers due to a singular system of equations. It does not matter if this PR is applied or not.

Ok, that is problematic. I assume the failure to simulate and assignment-error are related; indicating that the current version of the function doesn't work, and not some unrelated error in the example model.

I don't know the exact reason - is it related to calling GenerateEvents indirectly (i.e., one function with GenerateEvents calling another one)? If so it might be possible to rewrite it as:

  state := ThermodynamicState(phase=
         if ((h < bubbleEnthalpy(SaturationProperties(psat=p,Tsat=0)) 
          or (h > dewEnthalpy(SaturationProperties(psat=p,Tsat=0))) or (p > R134aData.data.FPCRIT)) then 1
         else 2 , p=p, h=h, d=density_ph(p, h), T=temperature_ph(p, h));

(I cannot test that in SimulationX.)

So what to do:

hubertus65 commented 5 months ago

I would be in favor to add the rewrite proposed by Hans. It makes the event generation explicit. Could you add it to the PR?

HansOlsson commented 5 months ago

I would be in favor to add the rewrite proposed by Hans. It makes the event generation explicit. Could you add it to the PR?

Should be done now. Please re-test

HansOlsson commented 2 weeks ago

@gkurzbach can you give some feedback? As I see it, the changes give clear benefits

casella commented 2 weeks ago

This PR should be back-ported to maint/4.1.0