search

DLR-SR / ThermofluidStream

The DLR Thermofluid Stream Library
BSD 3-Clause "New" or "Revised" License
55 stars 26 forks source link

Functions with wrong type argument called in new tank models? #195

Open casella opened 2 weeks ago

casella commented 2 weeks ago

Many of the recently added models, e.g. ThermofluidStream.Boundaries.Tests.TankExtendedTest2 fail in OMC with this error

[ThermofluidStream 1.1.0-main/Boundaries/Internal/partialTank.mo:126:3-127:39:writable]
Error: Type mismatch for positional argument 1 in 
ThermofluidStream.Boundaries.Tests.TankExtendedTest2.tank.Medium.Liquid.density(state=tank.medium.state). 
The argument has type:
  tank.Medium.ThermodynamicState
expected type:
  tank.Medium.Liquid.ThermodynamicState

This is the offending code: https://github.com/DLR-SR/ThermofluidStream/blob/38553ffdb79f8d65566b06c46249a14605eca1e9/ThermofluidStream/Boundaries/Internal/PartialTank.mo#L126-L129

where

https://github.com/DLR-SR/ThermofluidStream/blob/38553ffdb79f8d65566b06c46249a14605eca1e9/ThermofluidStream/Boundaries/Internal/PartialTank.mo#L3-L5

and

https://github.com/DLR-SR/ThermofluidStream/blob/38553ffdb79f8d65566b06c46249a14605eca1e9/ThermofluidStream/Boundaries/Internal/PartialTank.mo#L58

I'm not sure how Dymola handles this code, but as far as I understand, OMC is right to complain: tank.medium.state is a ThermodynamicState record from an instance of BaseProperties from package ThermofluidStream.Media.myMedia.GasAndIncompressible.JP8DryAir, while the function density() is called once from package Medium.Liquid, i.e. ThermofluidStream.Media.myMedia.Incompressible.Examples.JP8 and once from package Medium.Gas, i.e. ThermofluidStream.Media.myMedia.Air.DryAirNasa.

This cannot work: the density() function and the BaseProperties model instance should both be taken from the same package to be consistent, otherwise how could they compute the right density?

@perost, can you please double-check and comment?

As I understand, you should probably define PartialGasAndIncompressible.BaseProperties to contain two state records, e.g.

Gas.ThermodynamicState state_gas;
Liquid.ThermodynamicState state_liquid;

then, you could have something like

Modelica.Units.SI.Density liquidDensity=Medium.Liquid.density(medium.state_liquid) 
  "density of the liquid in the tank"; 
 Modelica.Units.SI.Density gasDensity=Medium.Gas.density(medium.state_gas) 
  "density of the gas in the tank";

of course PartialGasAndIncompressible.BaseProperties can also contain 

ThermodynamicState state;

but this state should contain the information about the overall properties of the gas-liquid mixture.

perost commented 2 weeks ago

@perost, can you please double-check and comment?

Yes, Medium.Liquid.density and Medium.Gas.density both expect a ThermodynamicState record that looks like:

record ThermodynamicState
  AbsolutePressure p;
  Temperature T;
end ThermodynamicState;

But medium.state is an instance of JP8DryAir.ThermodynamicState which looks like:

record ThermodynamicState
  AbsolutePressure p;
  Temperature T;
  MassFraction X[ns];
end ThermodynamicState;

I.e. it has an extra field X. The specification says that function arguments "must agree with the type of the corresponding parameter", which could mean pretty much anything. But we interpret it to mean that they must be type compatible as defined in 6.7, which states that the records must have the same named elements.

IngelaLind commented 2 weeks ago

Thank you for pointing this out, in Dymola there isn´t even a warning. I will look into it, but since it is outside my comfort zone, it might take a while to find a good solution. I will take your analysis into account.

casella commented 2 weeks ago

But we interpret it to mean that they must be type compatible as defined in 6.7, which states that the records must have the same named elements.

I also understood that. The mixture medium has two components, hence X[nS] is a two-dimensional array, while the liquid and gas have X[nS] containing a single scalar. Hence, they cannot be used interchangeably.

Bottom line: this probably works in Dymola in the specific case of two pure components, because you can compute the air and liquid properties as a function of p and T, so X[nS] does not matter. But I'd say this is a bit borderline, and looks to be working a bit like magic. I definitely suggest to define two separate ThermodynamicState records for the two phases, which can also be trivially extended to the case one or both of them are mixtures.

IngelaLind commented 2 weeks ago

I think the function call should be Modelica.Units.SI.Density liquidDensity=Medium.Liquid.density(Medium.Liquid.setState_pTX(medium.state.p,medium.state.T,{1})) "density of the liquid in the tank"; Modelica.Units.SI.Density gasDensity=Medium.Gas.density(Medium.Gas.setState_pTX(medium.state.p,medium.state.T,{1})) "density of the gas in the tank"; This simulates in Dymola. But that did the previous version as well, without any warnings at all, even with pedantic check. I have checked that the change works for both the directed and undirected version of the partialTank.

I only work part time and I do not have time to formally suggest the changes until earliest Thursday.

casella commented 2 weeks ago

Sounds good, it should also work in OMC. BTW, you should be able to skip the {1} input, as the setState_pTX function has a default input value reference_X for it.