Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modelica.Media conformance to the specification #1106

Closed
modelica-trac-importer opened this issue Jan 14, 2017 · 13 comments
Closed

Modelica.Media conformance to the specification #1106

modelica-trac-importer opened this issue Jan 14, 2017 · 13 comments
Assignees
Labels
discussion Discussion issue that it not necessarily related to a concrete bug or feature L: Media Issue addresses Modelica.Media P: highest Highest priority issue
Milestone

Comments

@modelica-trac-importer
Copy link

Reported by adrpo on 16 May 2013 06:16 UTC
Hi,

Just wondering if Modelica.Media is conform to the specification.

Spec says:

5.3.2 Composite Name Lookup
...
The class we look inside may not be partial in a simulation model.
package MoistAir 
  extends .Modelica.Media.Interfaces.PartialCondensingGases(
    mediumName = "Moist air", 
    fluidConstants = {IdealGases.Common.FluidData.H2O, IdealGases.Common.FluidData.N2}
    ...);

Now IdealGases.Common.FluidData looks like:

  constant PartialMixtureMedium.FluidConstants N2(
                       chemicalFormula =        "N2", ...);

Which uses PartialMixtureMedium.FluidConstants from a partial package.

I know that fluidConstants is not used in MoistAir at all, but that doesn't matter.

The question is *what* can you use from a partial package and *how*.
The specification is rather vague on this point.

Cheers,
Adrian Pop/


Migrated-From: https://trac.modelica.org/Modelica/ticket/1106

@modelica-trac-importer modelica-trac-importer added discussion Discussion issue that it not necessarily related to a concrete bug or feature L: Media Issue addresses Modelica.Media labels Jan 14, 2017
@modelica-trac-importer
Copy link
Author

Modified by sjoelund.se on 16 May 2013 19:53 UTC

@modelica-trac-importer modelica-trac-importer added this to the MSL3.2.1 milestone Jan 14, 2017
@modelica-trac-importer
Copy link
Author

Comment by otter on 17 May 2013 15:09 UTC
I needed a while to figure out the issue. Here is a copy of the relevant part of Modelica.Media:

package Interfaces
  package Types
    package Basic
      record FluidConstants 
         String iupacName;
         String casRegistryNumber;
         String chemicalFormula;
         String structureFormula;
         MolarMass molarMass;
      end FluidConstants;
    end Basic
  end Types

  partial package PartialMedium
     replaceable record FluidConstants =
         Modelica.Media.Interfaces.Types.Basic.FluidConstants;
     ...
  end PartialMedium;

  partial package PartialMixtureMedium
     extends PartialMedium(redeclare replaceable record FluidConstants =
        Modelica.Media.Interfaces.Types.IdealGas.FluidConstants);
     constant FluidConstants[nS] fluidConstants;      
     ...
  end PartialMixtureMedium

  partial package PartialCondensingGases
    extends PartialMixtureMedium(ThermoStates =
      Modelica.Media.Interfaces.Choices.IndependentVariables.pTX);
    ...
  end PartialCondensingGases

  package MoistAir
    extends Interfaces.PartialCondensingGases(
       mediumName="Moist air",
       substanceNames={"water","air"},
       final reducedX=true,
       final singleState=false,
       reference_X={0.01,0.99},
       fluidConstants={IdealGases.Common.FluidData.H2O,
                       IdealGases.Common.FluidData.N2},
end Interfaces;

package IdealGases
  package Common
     package FluidData
        import Modelica.Media.Interfaces.PartialMixtureMedium;     
        constant 
          PartialMixtureMedium.FluidConstants N2(ChemicalFormula="N2", ...);
        ...
     end FluidData
  end Common
end IdealGases

Now, the issue is that

   constant FluidConstants[nS] fluidConstants = 
                  {IdealGases.Common.FluidData.H2O,
                   IdealGases.Common.FluidData.N2},

refers to FluidData.N2 which is instantiated from a type in a partial package:

   constant PartialMixtureMedium.FluidConstants N2(...);

What is unclear to me is the difference of a "partial package" to a "package"?
Is there really a difference? For example, one easy fix would be to remove the keyword "partial" from all Interfaces.PartialXXXX packages, like PartialMixtureMedium.

Another fix would be (but seems to be unnecessarily complicated to:

  package FluidData
     package Medium = Modelica.Media.Interfaces.PartialMixtureMedium;     
     constant 
       Medium.FluidConstants N2(ChemicalFormula="N2", ...);
     ...
  end FluidData

If the computer science group agrees that there is no actual difference between a partial package and a package, I would prefer to clarify the specification by allowing name lookup in a partial package in a simulation model (but NOT in other partial classes).

@modelica-trac-importer
Copy link
Author

Comment by anonymous on 17 May 2013 15:44 UTC
This is very easy to fix without a change to the specification, using a type from a partial package is simply a mistake. I am running tests now and will update when done.

@modelica-trac-importer
Copy link
Author

Comment by hubertus on 17 May 2013 15:49 UTC
The last coment was from me, did not notice that I was not logged in.

@modelica-trac-importer
Copy link
Author

Comment by fcasella on 17 May 2013 15:56 UTC
I am not a computer science expert, but I think I understand the rationale of the design of the library, and I would like the language to support it.

Partial packages such as PartialMedium or PartialMixtureMedium provide the interface, and possibly an incomplete implementation, of a medium model. As such, they cannot be used directly in a simulation model, so they are marked as partial in order to get meaningful error reports if a user forgets to redeclare the medium package to be a complete implementation. It is much better to get a message such as: hey, your model declares a partial package, please redeclare it, than to get errors about some obscure implementation details being missing from the medium package. From this point of view, removing the "partial" keyword would imply a loss of possibly useful static debugging information.

When the usable (thus not partial) package is derived from the partial one, some of its classes might be modified or redeclared, but not necessarily all of them. If some of them (e.g., the FluidConstants record) are only declared in the base partial package, and they are not partial themselves, they should be usable. I cannot see any obvious reasons to forbid this, though I might be naive here.

If there is a way to fix the specification in order to allow this, without causing problems such as, e.g., substantially more involved lookup algorithms, I would definitely recommend to go for it.

@modelica-trac-importer
Copy link
Author

Comment by hubertus on 17 May 2013 16:02 UTC
Replying to [comment:5 fcasella]:

I am not a computer science expert, but I think I understand the rationale of the design of the library, and I would like the language to support it.

Partial packages such as PartialMedium or PartialMixtureMedium provide the interface, and possibly an incomplete implementation, of a medium model. As such, they cannot be used directly in a simulation model, so they are marked as partial in order to get meaningful error reports if a user forgets to redeclare the medium package to be a complete implementation. It is much better to get a message such as: hey, your model declares a partial package, please redeclare it, than to get errors about some obscure implementation details being missing from the medium package. From this point of view, removing the "partial" keyword would imply a loss of possibly useful static debugging information.

When the usable (thus not partial) package is derived from the partial one, some of its classes might be modified or redeclared, but not necessarily all of them. If some of them (e.g., the FluidConstants record) are only declared in the base partial package, and they are not partial themselves, they should be usable. I cannot see any obvious reasons to forbid this, though I might be naive here.

If there is a way to fix the specification in order to allow this, without causing problems such as, e.g., substantially more involved lookup algorithms, I would definitely recommend to go for it.

From my point of view it is fine to discuss htis down the road in the language group. This can be achieved without defining the type in a partial pacakge, and I updated Modelica.Media so that this is possible. This remaining problem was simply an oversight, which I just fixed.

@modelica-trac-importer
Copy link
Author

Comment by hubertus on 17 May 2013 16:04 UTC
Fixed in 7841ac1. Adrian, pleae r-open if you find further instances of the same problem.

@modelica-trac-importer
Copy link
Author

Comment by adrpo on 17 May 2013 16:47 UTC
Thanks Hubertus. I'll let you know if I find more of these.

You are right, I will create a ticket about this for the Modelica Specification.
Francesco: The only thing I want to know is what kind of checks and error messages should
you produce when ones uses a partial package in a path, i.e.:
NonPartial1.NonPartial2.Partial1.NonPartial3 (or any other combination)
Sometimes you can do full type checking of Partial1 here as you do have all the types,
but sometimes you can't as you have missing function redeclares, missing constant bindings, missing bindings for structural parameters (array dimensions, conditional components, condition in if equations). The question is also what is the checking scope here: you only check NonPartial3 and its dependencies and let all the other things in Partial1 be whatever (ignore even typing errors) or you check them all and warn about what is missing?

Cheers,
Adrian Pop/

@modelica-trac-importer
Copy link
Author

Comment by adrpo on 17 May 2013 17:38 UTC
From #1105 with more info.

Models:
Modelica.Media.Examples.RealGasAir.MoistAir uses
Modelica.Media.Air.RealGasMoistAir directly.
Models:
Modelica.Media.Examples.RealGasAir.MoistAir1 and Modelica.Media.Examples.RealGasAir.MoistAir1 redeclares medium to be:

redeclare package Medium = Modelica.Media.Air.RealGasMoistAir;

and Modelica.Media.Air.RealGasMoistAir contains:

package Modelica.Media.Air.RealGasMoistAir
  extends Modelica.Media.Interfaces.PartialRealCondensingGases(
    fluidConstants=
     {Modelica.Media.Water.IAPWS95.Water95_Base.waterConstants,
      Modelica.Media.Air.RealGasAir.Air_Base.airConstants});

Here both:
Modelica.Media.Water.IAPWS95.Water95_Base and
Modelica.Media.Air.RealGasAir.Air_Base
are partial packages.

Cheers,
Adrian Pop/

@modelica-trac-importer
Copy link
Author

Comment by hubertus on 17 May 2013 17:51 UTC
Stefan, Please pick up how the issue with partial types was changed for moist air. All Types are now defined in

 Modelica.Media.Interfaces.Types

Which is non-partial. If you use basic types and composite ones like FluidConstants from there, everything will be conformant to the specification. Please look at the issues reported by Adrian above.
I put this on severity blocker, because this is the last issue that I can see that is at least questionable from the point of view of MLS compliance. But: this is actually easy to fix you have the recipe.

@modelica-trac-importer modelica-trac-importer added the P: highest Highest priority issue label Jan 14, 2017
@modelica-trac-importer
Copy link
Author

Comment by adrpo on 17 May 2013 17:57 UTC
Just one note. My last comment is not an issue with types from partial packages.
Is the fact that constants from partial packages are used.
Modelica.Media.Water.IAPWS95.Water95_Base and
Modelica.Media.Air.RealGasAir.Air_Base are marked
as partial even if they do seem to be non-partial
packages.

Cheers,
Adrian Pop/

@modelica-trac-importer
Copy link
Author

Comment by otter on 18 May 2013 14:32 UTC
Replying to [comment:9 adrpo]:

From #1105 with more info.

Models:
Modelica.Media.Examples.RealGasAir.MoistAir uses
Modelica.Media.Air.RealGasMoistAir directly.
Models:
Modelica.Media.Examples.RealGasAir.MoistAir1 and Modelica.Media.Examples.RealGasAir.MoistAir1 redeclares medium to be:

redeclare package Medium = Modelica.Media.Air.RealGasMoistAir;

and Modelica.Media.Air.RealGasMoistAir contains:

package Modelica.Media.Air.RealGasMoistAir
  extends Modelica.Media.Interfaces.PartialRealCondensingGases(
    fluidConstants=
     {Modelica.Media.Water.IAPWS95.Water95_Base.waterConstants,
      Modelica.Media.Air.RealGasAir.Air_Base.airConstants});

Here both:
Modelica.Media.Water.IAPWS95.Water95_Base and
Modelica.Media.Air.RealGasAir.Air_Base
are partial packages.

Fixed in 7e19343:

Moved all FluidConstants xxxConstants definition from partial packages to the top level part of the medium (as in all the other media now) and instanciated from Types.xxx.FluidConstants instead of the type in the partial package. I hope that now all these issues are fixed (did this for ReferenceAir, RealGasAir, IAPWS95, IAPWS09.

Cheers,
Adrian Pop/

@modelica-trac-importer
Copy link
Author

Modified by otter on 18 May 2013 14:38 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion issue that it not necessarily related to a concrete bug or feature L: Media Issue addresses Modelica.Media P: highest Highest priority issue
Projects
None yet
Development

No branches or pull requests

3 participants