modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
459 stars 166 forks source link

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

Closed lvanfretti closed 4 years ago

lvanfretti commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

beutlich commented 4 years ago

See also #2515 for some previous analyses.

beutlich commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

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

casella commented 4 years ago

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:

I can do both if you want

lvanfretti commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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. :man_shrugging:

casella commented 4 years ago

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

casella commented 4 years ago

@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?