Issues with partial packages in Chemical library

[casella]

Please run https://libraries.openmodelica.org/branches/newInst/Chemical/files/Chemical_Chemical.Examples.FluidAdapter.err Chemical.Examples.FluidAdapter. The NF fails with

[Modelica 3.2.3/Media/package.mo:5463:7-5466:18:writable] Error: 
Called function ‘setState_pTX‘ is partial.
[Modelica 3.2.3/Fluid/Interfaces.mo:20:5-21:69:writable] Error:
Class Medium.ExtraProperty not found in scope FluidPort.

The model uses the medium package StandardWater_C, whose definition is

package StandardWater_C
 extends Modelica.Media.Water.StandardWater(
   extraPropertiesNames={"H2O"},
   singleState=true, T_default=310.15, X_default=ones(nX),  C_default={1000});

 extends Chemical.Interfaces.PartialMedium_C(
    redeclare package stateOfMatter = Chemical.Interfaces.Incompressible,
    ThermoStates=Modelica.Media.Interfaces.Choices.IndependentVariables.ph,
    mediumName="WaterIF97",
    substanceNames={"water"},
    fixedX=true,
    reference_p(start=50e5),
    reference_T(start=500),
    p_default(start=50e5),
    h_default(start=1.0e5),
    extraPropertiesNames={"H2O"},
    singleState=true,
    T_default(start=500) = 310.15,
    X_default=ones(nX),
    C_default={1000},
    substanceData={Examples.Substances.Water_liquid()});
end StandardWater_C;

This is a bit tricky: I understand the intention is to add some extra stuff (contained in PartialMedium_C) to the fully functional StandardWater. It seems that the second extends overrides the definitions of function of StandardWater and reinstates something from the partial model PartialMedium_C extends PartialMedium;.

Dymola runs this model without giving any warning about types.

@perost, is there something fishy in this medium definition, or is your partial-package detection still a bit too over-reacting?

[perost]

I don't think the model is valid, because different definitions of setState_pTX will be inherited through the two extends (since PartialMedium_C inherits directly from Modelica.Media.Interfaces.PartialMedium without redeclaring anything). If I look at the definition of the package in the library as it is right now the two extends clauses are actually reversed, so in case an element is defined in both packages the one from PartialMedium_C will be kept since it's first.

But it seems to me that the model should really be rejected even earlier due to StandardWater_C inheriting duplicated non-identical elements. Our checks for what is identical are not very good though, and while we try to at least check that the class prefixes are the same it seems we don't handle this exact case yet.

[marek@…]

It seems to be the diamond problem in multiple inheritance. Solution of it for extended packages is today incompatible between OpenModelica 1.16 and Dymola 2021. Both solution depends on order of classes after the "extends" keyword in each implementation different way.

Similar task of multiple inheritance was already solved here ​https://trac.modelica.org/Modelica/ticket/2015, which is in my opinion more close to Dymola implementation (to keep first defined class).

[perost]

We do keep the first defined class, since that's what the Modelica specification says. But that's currently the one in PartialMedium_C (the declaration in the ticket description is not the same as the one ​currently in the library). So you could "fix" it by swapping the two extends clauses, but you shouldn't be allowed to inherit different definitions of an element in the first place.

The only thing PartialMedium_C does in Chemical is to inherit from Modelica.Media.Interfaces.PartialMedium and define a stateOfMatter replaceable package. So you could make a separate package with stateOfMatter in it and extend from that in e.g. StandardWater_C instead, to avoid pulling in all the incompatible elements from PartialMedium.

[marek@…]

Soon I will change the order back, because the order PartialMedium_C,StandardWater does not work in Dymola (but this works in OpenModelica).

I want just to have an interface (PartialMedium_C), which extends MSL PartialMedium to use it as base interface in Chemical library. With combination of reuse defined medium of MSL I do not see another simple alternative to solve this pattern.

Multiple "constrainedby" could solve this pattern also, if it was supported by Modelica.

Of course - one of the solution is copy and modify MSL media to Chemical library, but ..

[casella]

Replying to perost:

We do keep the first defined class, since that's what the Modelica specification says. But that's currently the one in PartialMedium_C (the declaration in the ticket description is not the same as the one ​currently in the library). So you could "fix" it by swapping the two extends clauses, but you shouldn't be allowed to inherit different definitions of an element in the first place.

Yeah, this is what I first thought when I saw that definition.

The only thing PartialMedium_C does in Chemical is to inherit from Modelica.Media.Interfaces.PartialMedium and define a stateOfMatter replaceable package. So you could make a separate package with stateOfMatter in it and extend from that in e.g. StandardWater_C instead, to avoid pulling in all the incompatible elements from PartialMedium.

I understand this doesn't work, because @marek wants to have

model FluidAdapter_C
  "Adapter between chemical substances of one homogenous chemical solution and Modelica.Fluid package components of MSL 3.2, where substances are stored as molarities in expraProperties"

  outer Modelica.Fluid.System system "System wide properties";

  replaceable package Medium = Interfaces.PartialMedium_C constrainedby
  Interfaces.PartialMedium_C
      "Medium model"   annotation (choicesAllMatching=true);

which works with all possible extended medium packages. So you need a base class for them, as @marek noticed in comment:4

[casella]

I understand that, loosely speaking, the Modelica type system looks at what is inside a type, instead of explicitly looking at the inheritance tree. So if I have

model A
  Real x = 1;
end A;

model B
  Real x = 1;
  Real y = 2;
end B;

model C
  replaceable model M = A;
end C;

model D
  C c(redeclare model M = B);
end D;

I can say that B inherits from A even if there is no explicit extends A; clause in B. Hence, D is legal. In fact, I could compile it with OMC.

Based on this idea, you could do something like this. First define an abstract class for your PartialMedium_C

partial package PartialMedium_C
  extends Modelica.Media.Interfaces.PartialMedium;
  replaceable package stateOfMatter =
                        Chemical.Interfaces.StateOfMatter constrainedby
    Chemical.Interfaces.StateOfMatter
    "Substance model to translate data into substance properties"
     annotation (choicesAllMatching = true);
    constant Modelica.SIunits.MassFraction Xi_default[nXi];
    constant Modelica.SIunits.Density default_density;

    constant stateOfMatter.SubstanceData substanceData[nC]
      "Definition of the substances"
  annotation (choicesAllMatching = true);
end PartialMedium_C;

as you already did, and use it as a constraining class for your FluidAdapter_C. But then, when you define StandardWater_C, don't inherit from it, but rather reapply the extra features on top of StandardWater

package StandardWater_C
extends Modelica.Media.Water.StandardWater(
   extraPropertiesNames={"H2O"},
   singleState=true, 
   T_default=310.15,
   X_default=ones(nX),
   C_default={1000},
   ThermoStates=Modelica.Media.Interfaces.Choices.IndependentVariables.ph,
   reference_p(start=50e5),
   reference_T(start=500),
   p_default(start=50e5),
   h_default(start=1.0e5));
   constant Chemical.Interfaces.Incompressible.SubstanceData substanceData[nC] = 
    {Examples.Substances.Water_liquid()};
   constant Modelica.SIunits.MassFraction Xi_default[nXi]=ones(nXi);
   constant Modelica.SIunits.Density default_density = 1000;
end StandardWater_C;

As long as the things that you are adding are compatible with the base class, it's going to be fine. You don't need to write an explicit extends statement for that.

The only thing I'm not sure about is if you can leave default_density without a binding in the base class and then add it in the "virtually extended" one. I guess so. Maybe @perost can comment on that.

This is for sure much better than copying and modifying the Water model from MSL. Although that is technically and legally possible, it is for sure not the best possibile solution.

[casella]

@marek, after your updates to the library, the three FluidAdapter test cases work fine, see the ​regression report.

[perost]

It will only work until we fix our detection of incompatible duplicate elements though, so it's only a temporary workaround.

[casella]

Ah, ok, I thought my suggestion as per comment:6 had been implemented already. @marek, feel free to do so before we reintroduce those checks for good.