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.Fluid.Examples.HeatingSystem fails to simulate #3236

Closed
lvanfretti opened this issue Nov 21, 2019 · 16 comments · Fixed by #3249
Closed

Modelica.Fluid.Examples.HeatingSystem fails to simulate #3236

lvanfretti opened this issue Nov 21, 2019 · 16 comments · Fixed by #3249
Assignees
Labels
example Issue only addresses example(s) L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation)
Milestone

Comments

@lvanfretti
Copy link
Member

lvanfretti commented Nov 21, 2019

Hi,
I am using Dymola 2020 with MSL v 3.2.3.

I have found that when simulating the example Modelica.Fluid.Examples.HeatingSystem, several errors are issued that are due to the medium.

I have done the following, I modified the line:
replaceable package Medium = Modelica.Media.Water.StandardWater
To the following medium:
replaceable package Medium = Modelica.Media.Water.ConstantPropertyLiquidWater
While I am not sure if this is the same medium, I was able to simulate the model successfully.

I can commit a pull request if this is acceptable.

Luigi

@dietmarw dietmarw added L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation) bug Critical/severe issue labels Nov 22, 2019
@dietmarw dietmarw modified the milestones: MSL3.2.3+maint, MSL4.0.0 Nov 22, 2019
@dietmarw
Copy link
Member

I can confirm this also for the current master version. A possible fix should also got into maintenence. @hubertus65 and @casella the proposed fix sounds reasonable, do you agree?

I've found several other occurrences where the medium is redefined using the package containing medium variants instead of the explicit medium. These should probably be fixed at the same time, right?

@lvanfretti
Copy link
Member Author

@dietmarw the issue that I had is that I was not reproduce the same results when making the change.

I tested using Dymola 2018 with MSL 3.2.3, and I figured out that the correct change we would need to do to keep things consistent is the one in "StandardWater=WaterIF97_ph".

Then, I tested in Dymola 2019 and Dymola 2020, and I found that to make the correct change then we need the following:
replaceable package Medium = Modelica.Media.Water.WaterIF97_ph

I don't understand why this is a problem, as I am using MSL 3.2.3 in all versions of Dymola, but perhaps the one used in Dymola 2018 is older (probably), and the changes made in the MSL would break the examples.

@dietmarw
Copy link
Member

I leave that to the library officers since not really my domain. :-)

@beutlich
Copy link
Member

See also #2515 for some previous analyses.

@beutlich
Copy link
Member

beutlich commented Nov 24, 2019

Then, I tested in Dymola 2019 and Dymola 2020, and I found that to make the correct change then we need the following:
replaceable package Medium = Modelica.Media.Water.WaterIF97_ph

Hm, cannot confirm. Also in Dymola 2020 I get the same "Error in region computation" when using WaterIF97_ph instead of StandardWater.

(Using ConstantPropertyLiquidWater succeeds to simulate, though I did not check the results.)

@hubertus65
Copy link
Member

From the point of view of "adequate" water model for a heating system, ConstantPropertyLiquidWater is probably better than WaterIF97_ph. But that would change results and is in principle not "backwards compatible", even if one could argue that it does not matter (that much) in an example. WaterIF97_pT is another option, also probably better than WaterIF97_ph with probably almost non-measureable differences. I would prefer to move to ConstantPropertyLiquidWater (also fewer states), if others agree.

@hubertus65
Copy link
Member

Ok, I looked at this in more detail (but with Dymola 2019FD1, not Dymola 2020, which I don't have on this computer. Here are my findings so far:

  • The fastest, and probably most appropriate Medium is Modelica.Media.Water.ConstantPropertyLiquidWater. Since the density is constant, the mass balance vanished through index reduction, and only temperature states remain. That makes sense for a water heating (unless you want to look at what an expansion tank does in detail.
  • Also an option, but causing an unnecessary dynamic state selection in Dymola are both Modelica.Media.CompressibleLiquids.LinearWater_pT_Ambient and Modelica.Media.CompressibleLiquids.LinearColdWater. In spite of the state selection glitch in Dymola, they would be simpler and more similar to the original model, and one could look at e.g. sizing an expansion tank.
  • The model can fail with Modelica.Media.Water.WaterIF97_ph because we ask for a steady-state initialization, and they are not robust against minor changes in the symbolic machinery in Dymola. Probably @HansOlsson is best suited to find an initialization that works in the last 4 Dymola versions, but preferably also in other tools. Another option is to not initialize in steady-state, which could be more robust across many tools.

All said and done, I'd go with the constant properties in the future (MSL 4.0.0) and update the reference properties, but that should not be done for MSL 3.2.3, where maybe @HansOlsson can find an initialization that holds across versions?

@hubertus65
Copy link
Member

@lvanfretti: your pull request is fine for MSL 4.0.0, which this ticket is filed under.

@hubertus65 hubertus65 modified the milestones: MSL4.0.0, MSL3.2.3+maint Nov 25, 2019
@casella
Copy link
Contributor

casella commented Nov 26, 2019

The HeatingSystem model has a serious issue, which is extensively discussed in OpenModelica ticket #5452.

To summarize, the pump model is algebraic, but for some reason the parameters of that component were set to have stateSelect = StateSelect.prefer. This apparently makes the problem of choosing a good (static!) state selection very difficult. OpenModelica had lots of trouble with this model because we got a dynamic state selection which led to very inefficient simulation.

One could keep this model as a very difficult test case for tools, but then the right place would be ModelicaTest, not Examples. I think the role of Examples packages is to contain simple example problems that show how to effectively use a library to solve simple modelling problems, not to make them challenging for the tools.

My recommendation is thus to apply the following changes to both MSL 3.2.3 maintenance and 4.0.0:

  • switch to a simpler medium model. IF97 is definitely too much for such a low temperature, low pressure system, I'm not sure if we want a compressible or incompressible fluid model. Maybe we could have them both, first compressible, then we extend it and change the fluid model to incompressible, so you can see you have less states, and a non-stiff system
  • fix the StateSelect flag of the pump so that tools don't even try to use the pump pressure or enthalpy as state, which is just stupid

I can do both if you want

@lvanfretti
Copy link
Member Author

@beutlich @casella @hubertus65
Thank you all for putting your energy in such a small system!
Since I am not really a domain expert here, I can't really pitch in anymore than what I have already.
So, I will let you guys come to a conclusion, and I think either of the three of you should to the PR for the final choice.

I'd like to remind you of the issue that @dietmarw mentioned above, which would be an important follow up and perhaps a different issue/PR:
I've found several other occurrences where the medium is redefined using the package containing medium variants instead of the explicit medium. These should probably be fixed at the same time, right?

So that similar issues do not arise in other models, I suggest we dig through and find/correct those models also.

@beutlich
Copy link
Member

I can do both if you want

@casella Please only provide a PR for master branch. I can care for the back-porting to maint/3.2.3 once it got merged.

I suggest we dig through and find/correct those models also.

@casella @hubertus65 Please also have a look why variants packages are assigned as medium packages.

Thank you all!

@HansOlsson
Copy link
Contributor

HansOlsson commented Nov 27, 2019

Regarding the stateSelect flag Dymola (since some versions, at least since Dymola 2019 I think) gives the following warning for the model:

We cannot differentiate equations for the following variables having stateSelect = StateSelect.prefer.
They will be treated as if they had stateSelect = StateSelect.default:
pump.medium.h
pump.medium.p

As far as I recall previous Dymola versions did something similar - but without the warning, and at least pump.medium.h is time-varying.

However, I also realize that several of the statements above regarding Dymola are unclear, and as far as I can tell:

Dymola 2020 (and Dymola 2020x) with either MSL 3.2.2 or MSL 3.2.3 generate recoverable errors during model initialization, but then recover and simulate the model to completion. Thus the model is sort of working, and even if it is good to use a different medium it may not needed for maintenance releases.

@casella
Copy link
Contributor

casella commented Nov 27, 2019

I agree with @HansOlsson that the model sort of works in Dymola as it is now.

My point is that this is really a simple model of an elementary illustrative system, so there is really no reason why it should end up being difficult to solve by any tool. Since the model was created using Dymola (I did it myself, if I'm not mistaken), it works with that tool by definition, but we should try to include in the MSL examples only models that are robust and not made artificially difficult to solve by inappropriate selection of parameters.

I think the MSL 4.0.0 clean up is a good occasion to improve that, and I also agree with @beutlich that we should port this to 3.2.3 maintenance, since this will be used for a while until the transition to 4.0.0 is over.

@dietmarw
Copy link
Member

I've found several other occurrences where the medium is redefined using the package containing medium variants instead of the explicit medium. These should probably be fixed at the same time, right?

You can ignore this. I misread the example. As I pointed out earlier not really my domain. 🤷‍♂️

@casella
Copy link
Contributor

casella commented Nov 28, 2019

OK. I was almost going to ask to explain what you meant with your comment, but you got there first 😄

@casella
Copy link
Contributor

casella commented Nov 28, 2019

@hubertus65 I need your advice not to screw things up, because the fix may have side effects on other models.

The pump model extends PartialLumpedVolume, which instantiates a BaseProperties medium model with preferredMediumStates = true in all cases. This is the root cause of the issue with the pump, but potentially with other models that use the same class, so I am inclined at fixing the base class, rather than just the pump model.

An easy fix is to do something like this in PartialLumpedVolume:

Medium.BaseProperties medium(
  preferredMediumStates = (if energyDynamics == Dynamics.SteadyState and massDynamics == Dynamics.SteadyState then false else true),
...

this doesn't cover the cases where energy is dynamic but mass is static, but that can't be done with the current design of preferredMediumStates anyway.

My idea would be to try this with MSL 4.0.0, check if there are regressions with tools, and then if we don't get into trouble port this to MSL 3.2.3 maintenance.

What do you think?

beutlich added a commit that referenced this issue Dec 9, 2019
Fix issue #3236 by using a simpler medium model for the HeatingSystem example and by improving state selection in PartialLumpedVolume
@beutlich beutlich added example Issue only addresses example(s) and removed bug Critical/severe issue labels Dec 9, 2019
beutlich pushed a commit to beutlich/ModelicaStandardLibrary that referenced this issue Dec 10, 2019
medium.preferredMediumStates is now false if both mass and energy balances are static
Attempt to fix issue modelica#3236
beutlich added a commit that referenced this issue Dec 11, 2019
… example and by improving state selection in PartialLumpedVolume (#3260)

* Changed medium model in HeatingSystem to simpler linear water model

* medium.preferredMediumStates is now false if both mass and energy balances are static

* Attempt to fix issue #3236
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example Issue only addresses example(s) L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation)
Projects
None yet
6 participants