lbl-srg / modelica-buildings

Modelica Buildings library
250 stars 155 forks source link

Invalid model because of lookup in partial class #2229

Closed casella closed 3 years ago

casella commented 3 years ago

Please check Buildings.Fluid.HeatExchangers.DXCoils.BaseClasses.Examples.NominalCondition. OpenModelica reports that

[Buildings 7.0.0/Fluid/HeatExchangers/DXCoils/BaseClasses/NominalCondition.mo:36:3-40:32:writable] 
Error: Medium is partial, name lookup is not allowed in partial classes.

The package Medium is declared to be a replaceable partial class, but is not subsequently redeclared to a non-partial one before being used. This is forbidden by the Modelica Specification, Section 5.3.2: "The class we look inside may not be partial in a simulation model".

mwetter commented 3 years ago

Correct, a non-partial media need to be assigned.

casella commented 3 years ago

When you fix this, can you pls. back-port it to a 7.0.1 maintenance branch, so we can refer to that for our work?

mwetter commented 3 years ago

Would a 7.0.1 branch be helpful for you, otherwise we would not likely do one but rather focus on getting 8.0.0 out, which has already a lot of fixes.

mwetter commented 3 years ago

@casella : I think we have a bigger problem: Consider

class Cl

  model A
    B myB(redeclare package Medium = Modelica.Media.Air.MoistAir);
  end A;

  model B
    replaceable package Medium = Modelica.Media.Interfaces.PartialMedium;

    // The statement below is not valid, and leads to an error in checking the model B in OMEdit.
    parameter Modelica.SIunits.SpecificEnthalpy h_default = Medium.specificEnthalpy_pTX(
      p=Medium.p_default,
      T=Medium.T_default,
      X=Medium.X_default) "Default enthalpy for the selected medium";
  end B;

end Cl;

which fails the model check for B in OMEdit. Also, Modelica.Fluid.Vessels.ClosedVolume fails to check, as does Pipes.StaticPipe, Valves.ValveLinear and probably many others in the MSL.

The above pattern is used throughout Modelica.Fluid.

casella commented 3 years ago

Would a 7.0.1 branch be helpful for you, otherwise we would not likely do one but rather focus on getting 8.0.0 out, which has already a lot of fixes.

See email

casella commented 3 years ago

@casella : I think we have a bigger problem: Consider

class Cl

  model A
    B myB(redeclare package Medium = Modelica.Media.Air.MoistAir);
  end A;

  model B
    replaceable package Medium = Modelica.Media.Interfaces.PartialMedium;

    // The statement below is not valid, and leads to an error in checking the model B in OMEdit.
    parameter Modelica.SIunits.SpecificEnthalpy h_default = Medium.specificEnthalpy_pTX(
      p=Medium.p_default,
      T=Medium.T_default,
      X=Medium.X_default) "Default enthalpy for the selected medium";
  end B;

end Cl;

which fails the model check for B in OMEdit. Also, Modelica.Fluid.Vessels.ClosedVolume fails to check, as does Pipes.StaticPipe, Valves.ValveLinear and probably many others in the MSL.

The above pattern is used throughout Modelica.Fluid.

That is a problem with the 'Check' feature, which tries to instantiate the model, and of course it can't because it still refers to partial models. We should introduce some relaxations so that instantiation of partial stuff proceeds, see #6217.

Then, we should also report that the model is locally balanced, despite some equations potentially being missing from the partial class. This is covered by #3977, which is not currently our highest priority.

mwetter commented 3 years ago

@casella : In view of https://trac.openmodelica.org/OpenModelica/ticket/6217, I assume this won't need to be fixed in the Buildings library for the maint_7.0.x branch, correct?

casella commented 3 years ago

@mwetter, it will still need to be fixed.

mwetter commented 3 years ago

@casella : I am not sure we talk about the same: Once https://trac.openmodelica.org/OpenModelica/ticket/6217 is fixed, then models like the example Cl will work, and hence also Buildings 7.0.0/Fluid/HeatExchangers/DXCoils/BaseClasses/NominalCondition.mo should work, as will the models in Modelica.Fluid, which currently (almost all?) fail to check with OMEdit.

Hence, I think keeping models with the pattern as in the example Cl is fine. If not, 100's of models will need to be changed, and the whole design principle of .Fluid in which a user has to redeclare the medium prior to simulation won't work.

Consequently, I would think we don't need to apply this patch to the maint_7.0.x branch. If you prefer to apply the patch so other tests can proceed, I'd be happy to do so.

casella commented 3 years ago

@casella : I am not sure we talk about the same

Neither am I 😄

Once https://trac.openmodelica.org/OpenModelica/ticket/6217 is fixed, then models like the example Cl will work

That depends on your definition of work.

Cl is a technically a model, but it only contains class definitions; there are no components, variables, or equations in it. As such, it is valid and can be legally compiled it into a simulation executable, but it does not compute anything, so it is rather useless as such. In fact, you can both check and compile Cl successfully with today's OMC, and what you'll get out of this is nothing, as expected.

I guess it makes more sense to consider Cl as a package instead, and then check and compile its models Cl.A and Cl.B.

Component models in the Modelica.Fluid library are like Cl.B, so they can't be simulated as such, but they need their partial medium model to be redeclared.

That said, my original diagnostic on Buildings.Fluid.HeatExchangers.DXCoils.BaseClasses.Examples.NominalCondition was wrong, my fault. That model declares a parameter nomCon of type Buildings.Fluid.HeatExchangers.DXCoils.BaseClasses.NominalCondition, but indeed it redeclares the partial medium package, so I guess there is something wrong with OMC that does not recognize this.

I opened #6221 to address this issue.

mwetter commented 3 years ago

@casella : Great, now we both agree. In this case I will leave maint_7.0.x as is.

casella commented 3 years ago

Yep. Maybe it could still get useful later, when we compare the results.

casella commented 2 years ago

@mwetter I question the label "OpenModelica" here. This issue was because of code which is not compilant to the Modelica Specification. You can't blame OpenModelica because it follows the rules 😃