Refactor WaterIF97_base and WaterIF97_fixedregion

[modelica-trac-importer]

Reported by fcasella on 12 Mar 2013 15:11 UTC
Modelica.Media.Water.WaterIF97_base and Modelica.Media.Water.WaterIF97_fixedregion have lots of code in common, so it's hard to spot the differences and maintain the code. They should probably be refactored, so that WaterIF97_fixedregion extends WaterIF97_base and just redeclares what is needed.

---

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

[modelica-trac-importer]

Comment by fcasella on 25 Mar 2013 10:06 UTC
While doing this, WaterIF97_fixedregion should probably be fixed, in order to make sure that a single region, specified by the package constant Region, is always assumed for all computations. In particular:

• the setState_XXX() functions should not have an extra region input (which is not part of the PartialTwoPhaseMedium interface, BTW), but rather use the Region package constant
• the setState_XXX() functions should determine the phase based on the known region, rather than by calling functions such as phase_ps(), which would cause a waste of time

[modelica-trac-importer]

Comment by beutlich on 29 Oct 2015 20:26 UTC
@fcasella and @HUBERTUS: This is one of the two open high-priority tickets on MSL 3.2.2 milestone. Do you think you can manage the refactoring in due time for MSL 3.2.2 release which is expected by end of 2015? Thanks for your contributions.

[modelica-trac-importer]

Comment by hubertus on 29 Oct 2015 21:15 UTC
The honest answer for myself is no, I don't have the time. But I will check if someone else within Modelon can take a look at this. I'll get back, ....

Francesco?

[modelica-trac-importer]

Comment by fcasella on 29 Oct 2015 22:35 UTC
I can manage to find some time to do this, it's not a difficult task. Some help from Modelon would be appreciated, e.g. for testing and double-checking. Is that possible?

[modelica-trac-importer]

Comment by hubertus on 2 Nov 2015 12:02 UTC
That is possible for sure.

[modelica-trac-importer]

Comment by hubertus on 7 Dec 2015 14:05 UTC
Reminder, ...

[modelica-trac-importer]

Comment by fcasella on 19 Dec 2015 01:35 UTC
I have done a first analysis of the task. The package Modelica.Media.Water.WaterIF97_fixedregion as it is now also supports setting Region = 0, which means that the region is unknown and needs to be computed. Supporting this option makes the code quite involved.

In fact, Modelica.Media.Water.WaterIF97_fixedregion(Region = 0) is completely equivalent to Modelica.Media.Water.WaterIF97_base, so there I don't see the point of handling this case twice with duplicate code. The rationale for Modelica.Media.Water.WaterIF97_fixedregion is that it should be used as a plug-in replacement for Modelica.Media.Water.WaterIF97_base when the region is known, in order to save time spent in useless checks to determine the region. Therefore, the region has to be known a-priori, and the interface of all the functions of Modelica.Media.Water.WaterIF97_fixedregion should be the same as inModelica.Media.Water.WaterIF97_base

I would then propose to exclude the Region = 0 case. Although strictly speaking this is a non-backwards compatible change, it does not affect any of the derived packages in the MSL, which of course set a well-defined region, corresponding to a positive value of the package constant Region. I think it is highly unlikely that anyone has actually used the Modelica.Media.Water.WaterIF97_fixedregion base class directly and set Region = 0, as there is really no reason to do so, since one can just use Modelica.Media.Water.WaterIF97_base instead in that case. Of course we should mention this in the release notes, just in case someone did.

Hubertus, if you give me the green light, I can commit the code to a branch for a quick inspection by you or someone from Modelon, and have it ready on trunk before the feature freeze on Dec 23.

A further issue I raised in comment:1 is that I don't see the point of having an extra input Region for the setState_XX and density_ph, density_pT etc. functions, which is not defined in the PartialTwoPhaseMedium interface. The region considered should just be the one set by the Region package constant, and the interface of those functions should be the same as in the base case. I can keep the code as it is, and just add a default binding region = Region for those inputs to the package constants, but this seems to me a useless complication. BTW, as these functions are defined now, with region = 0 by default, the package is completely useless, because all the checks will be always performed in all those functions if one just plugs in a fixedregion medium in place of the base one, even if the package constant Region is set to a positive value.

If you agree, I would also remove this feature from the Modelica.Media.Water.WaterIF97_fixedregion, and set the region input of the low level function straight to the Region package constant.

I stand by for your feedback in order to proceed.

[modelica-trac-importer]

Comment by hubertus on 19 Dec 2015 03:59 UTC
Francesco, please go ahead. I agree with all your suggestions. I'll look through this after you have committed this.

[modelica-trac-importer]

Comment by fcasella on 23 Dec 2015 18:42 UTC
It is actually going to be a bit more complicated than I thought. My original plan was to extend the _fixedregion package from the _base one and redeclare only what was necessary. Unfortunately, in _fixedregion I cannot undeclare stuff (e.g., BaseProperties equations) which was declared in _base, and if I extend from the base classes in Media I end up in type hell.

So, the refactoring requires to first define a _common base class with all the shared stuff, and then have _base and _fixedregion both extend from that, adding specific code. Maybe I can rewrite _base to also act as _common with the default Region = 0 choice, I have to check that.

I'll attend to this asap. As this is considered as a bugfix and not a new feature, there is time until Jan 12 to review it.

[modelica-trac-importer]

Comment by fcasella on 4 Jan 2016 01:56 UTC
It turns out the solution is quite easy. The original _base package functions did not use the region input of all low-level functions, which then defaults to zero. If the package constant Region = 0 is added to the _base package, then the _fixedregion low level function implementations can be used also for the _base package, with the same semantics. As a consequence, the _fixedregion implementation can be substituted by the simple extend statement

partial package WaterIF97_fixedregion
  "Water: Steam properties as defined by IAPWS/IF97 standard, fixed region"
  extends WaterIF97_base(Region(min=1)=1);
end WaterIF97_fixedregion;

which simply bars using Region = 0.

This is immediately apparent by looking at the diff between 0841762 and the previous revision, whereby a large chunk of code of the _base function is removed and replaced by the original implementations inside _fixedregion. It was also necessary to add one more case to the BaseProperties equations to handle the phase variable in case of fixed region selection, i.e., Region > 0.

Two important fixes were also introduced.

The first is that the original _fixedregion package did not work as expected when used as a plug-in replacement for the WaterIF97_ph package, because the package constant Region was ignored, and it was expected that the extra input Region be set explicitly by the calling model. Of course this didn't work because the calling model is not changed at all if only the package is redeclared. This has now been fixed by having all the package functions use the package constant Region as a default region input.

The second fix concerns the phase field in the setState_dTX, phX, and psX functions. The _base functions contained the statment phase = 0;, while the _fixedregion functions had something like phase = if region == 0 then IF97_Utilities.phase_ph(p, h). Since these functions are meant to be inlined, the latter implementation eventually brings to an Integer equation with conditions based on Real variables outside a when clause, which cannot be handled. I have thus kept the phase = 0 implementation in the new _base package functions.

Some tests with ThermoPower in Dymola reveal that the new implementation of the _fixedregion for a liquid water heat exchanger brings to a speed-up of almost a factor 2, when WaterIF97_R1ph is used instead of WaterIF97_ph, while the previous implementation actually failed for the above-mentioned reason.

The examples in Modelica.Fluid using the water medium seem to work fine. It would be good to do some more testing with other libraries, botht with the standard water package and with the fixed region ones.

[modelica-trac-importer]

Comment by fcasella on 4 Jan 2016 01:57 UTC
Fixed in 0841762

[modelica-trac-importer]

Modified by beutlich on 4 Jan 2016 13:24 UTC

[modelica-trac-importer]

Changelog modified by beutlich on 4 Jan 2016 13:24 UTC
Refactored WaterIF97_base and WaterIF97_fixedregion