Add wall temperature and heat accumulation terms to the 0D heat exchanger #714

rustygentile · 2022-02-17T21:24:16Z

Fixes

Summary/Motivation:

Adding a new unit model that extends the 0D HeatExchanger by including wall temperature and heat holdup terms in the overall energy balance. This will improve transient simulations.

Changes proposed in this PR:

Adding HeatExchangerLumpedCapacitance unit model class
Adding unit tests for the new model
Adding a readthedocs page that includes an example with sample code

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

…dated failing regression test

…id_dynamic() and save to file

andrewlee94

From an initial read through, this looks really good. My only comments were a couple of minor things.

andrewlee94 · 2022-02-18T15:02:41Z

idaes/generic_models/unit_models/heat_exchanger_lc.py

+
+    def initialize(self, *args, **kwargs):
+
+        if self.flowsheet().config.dynamic:


My initial though is that this should use the local self.config.dynamic flag - is there a reason this will not work?

The reason for this is that I want to use control volumes with dynamic=False. And the way the base class is written, the control volumes inherit their dynamic flags from the unit model config. In heat_exchanger.py we have:

hot_side = _make_heater_control_volume( self, config.hot_side_name, config.hot_side_config, dynamic=config.dynamic, has_holdup=config.has_holdup, )

What you're suggesting would call for a config like so:

m.fs.unit = HeatExchangerLumpedCapacitance(default={ "shell": {"property_package": m.fs.properties, "dynamic": False}, "tube": {"property_package": m.fs.properties, "dynamic": False}, "dynamic": True})

And then, for instance, in heat_exchanger.py:

try: hot_dynamic = hot_side_config.dynamic except KeyError: hot_dynamic = config.dynamic hot_side = _make_heater_control_volume( self, config.hot_side_name, config.hot_side_config, dynamic=hot_dynamic, has_holdup=config.has_holdup, )

That would give us three different levels where the dynamic flag could be configured: flowsheet -> unit model -> control volume. And in that case, we should probably do the same for has_holdup. Actually, the new logic probably belongs in _process_config().

I'd be happy to try this, but I thought it best not to make any changes to heat_exchanger.py in case someone else is also working on it. Let me know which you prefer, to leave it as-is, or to make the changes to both unit models and have dynamic be configurable at each level.

I am not sure I follow here. Are you saying that you always want the control volumes to be steady-state (i.e. dynamic=False) and even thought the unit model (and flowsheet) might be dynamic? If so, this is not the way to do it, as currently the dynamic flag is inherited by default and you want to over-ride that (which is currently not happening).

If that is what you want to do, then I can see two ways of going about this.

The more correct way would be to not inherit from the heat exchanger model at all and build an entirely new model (as the difference, whilst relatively small, are unfortunately far reaching). In doing so, you could decouple the control volume dynamic flag form the unit model dynamic flag.

The second way, which would be simpler but still have the disconnect between unit and flowsheet, would look much like what you already have, but you would need to change the unit level configuration argument for the dynamic flag (and the has_holdup flag). What you would need to do is change their settings so that the default value is False and that the valid domain is also False - that forces the unit model itself to always be steady-state (and prevents it from inheriting from the flowsheet).

Are you saying that you always want the control volumes to be steady-state (i.e. dynamic=False) and even thought the unit model (and flowsheet) might be dynamic?

I don't want them to always be steady-state. But I'd like to leave that in as an option. With the example that's included, I'm essentially assuming that while there's heat holdup in the wall material, any mass holdup in the working fluid is negligible. So the way it's implemented now, the dynamic and has_holdup flags only affect the control volumes.

The more correct way would be to not inherit from the heat exchanger model at all and build an entirely new model (as the difference, whilst relatively small, are unfortunately far reaching).

I was afraid it might come to that. That sounds like the most thorough approach. The downside though, would be lots of duplicate code.

First, I think we need to work out exactly what you want to allow for, as I am not quite sure what is happening. Is it that you want to allow separate the decision about holdup dynamics from that of the wall energy balance then - i.e. allow the wall energy balance to be dynamic even if the holdups are steady-state?

Is it that you want to allow separate the decision about holdup dynamics from that of the wall energy balance then - i.e. allow the wall energy balance to be dynamic even if the holdups are steady-state?

Yes exactly!

OK, my first thought then is that we should allow the user to choose wall dynamics separately from the material balance - I.e. have a separate configuration option for the wall dynamics that the user can set, and then have some logic on the default setting (much like how the material dynamics are done).

We might want to think about any combinations that do not make sense however - I am not sure that there are any however (at most it would be setting everything to steady-state, in which case this devolves into the basic heat exchanger, and that will still work).

OK, my first thought then is that we should allow the user to choose wall dynamics separately from the material balance - I.e. have a separate configuration option for the wall dynamics that the user can set, and then have some logic on the default setting (much like how the material dynamics are done).

This is a nice solution. If there are no other concerns or objections, I'll go ahead and implement it. Do you have a preference for the new variable name? I was thinking either dynamic_wall or dynamic_heat_balance. Also, are there any other similar unit models? Because it might be good to standardize or reuse this variable name if possible.

We might want to think about any combinations that do not make sense however - I am not sure that there are any however (at most it would be setting everything to steady-state, in which case this devolves into the basic heat exchanger, and that will still work).

Agreed. That did occur to me. My initial thought was that there wasn't a use case for having everything be steady-state, since one could just use the base heat exchanger model. But with this model, you at least get the wall temperature in addition. So that might still be useful to have. Especially if you're troubleshooting a flowsheet or model that's difficult to solve, it could be helpful to see this temperature at steady-state.

Also, if you're in "troubleshooting mode" I think it's better to change something with a flag rather than swapping out the entire unit model. Since using a different unit model introduces more unknowns. So I think it's important to make sure this model still works with steady-state flowsheets.

The same argument could be made for the other config combinations. The user might want to make sure their model solves with dynamic control volumes before activating the wall dynamics. And vice versa too.

dynamic_heat_balance is the best of anything I can think of so far.

dynamic_heat_balance is the best of anything I can think of so far.

Ok done! I also added a test to make sure each combination of dynamic / dynamic_heat_balance can be built. And some xfail-ing tests for invalid configurations.

idaes/generic_models/unit_models/tests/test_heat_exchanger_lc.py

docs/reference_guides/model_libraries/generic/unit_models/heat_exchanger_lc.rst

idaes/generic_models/unit_models/heat_exchanger_lc.py

codecov · 2022-02-18T15:50:08Z

Codecov Report

Merging #714 (02e5acf) into main (f2adca6) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #714      +/-   ##
==========================================
+ Coverage   64.58%   64.77%   +0.18%     
==========================================
  Files         371      375       +4     
  Lines       59294    60130     +836     
  Branches    10923    11088     +165     
==========================================
+ Hits        38293    38947     +654     
- Misses      19049    19195     +146     
- Partials     1952     1988      +36

Impacted Files	Coverage Δ
idaes/generic_models/unit_models/__init__.py	`100.00% <100.00%> (ø)`
...es/generic_models/unit_models/heat_exchanger_lc.py	`100.00% <100.00%> (ø)`
idaes/ver.py	`61.53% <0.00%> (-4.62%)`	⬇️
...c_models/properties/core/state_definitions/FTPx.py	`83.76% <0.00%> (-3.38%)`	⬇️
idaes/dmf/cli.py	`65.38% <0.00%> (-2.69%)`	⬇️
...daes/apps/uncertainty_propagation/uncertainties.py	`82.10% <0.00%> (-2.34%)`	⬇️
idaes/dmf/util.py	`54.04% <0.00%> (-1.62%)`	⬇️
idaes/dmf/resource.py	`72.32% <0.00%> (-1.53%)`	⬇️
idaes/dmf/dmfbase.py	`74.57% <0.00%> (-1.08%)`	⬇️
idaes/surrogate/keras_surrogate.py	`87.50% <0.00%> (-0.74%)`	⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2adca6...02e5acf. Read the comment docs.

…t to docs page, removed unneeded CONFIG line in heat_exchanger_lc

…d capacitance config

andrewlee94

Looking good. I just had a couple of suggestions on how to improve the usability of the new config argument and on a way to replace some of the xfails with tests that will check the error messages.

andrewlee94 · 2022-03-04T14:24:44Z

idaes/generic_models/unit_models/tests/test_heat_exchanger_lc.py

+            "shell": {"property_package": m.fs.properties},
+            "tube": {"property_package": m.fs.properties},
+            "dynamic": False,
+            "dynamic_heat_balance": True})


For these cases, we generally prefer the following to an xfail:

with pytest.raises(DAE_Error, match="error message"): m.fs.unit = HeatExchangerLumpedCapacitance(default={ "shell": {"property_package": m.fs.properties}, "tube": {"property_package": m.fs.properties}, "dynamic": False, "dynamic_heat_balance": True})

The reason being that this is actually a successfully test of an expected failure, so this way the test shows up as passed, plus it also checks that the error message is what was expected.

That makes sense. I didn't do this for the InconsistentUnitsError though, since that's not something we really intended. And this doesn't seem to be caused by IDAES. So I'll try and setup a simple example and submit an issue to the Pyomo folks.

@rustygentile Pyomo is already aware of the issue with units: Pyomo/pyomo#1790. However, more people commenting about this being a problem might help motivate a fix.

andrewlee94 · 2022-03-04T14:29:10Z

idaes/generic_models/unit_models/heat_exchanger_lc.py

+    CONFIG.declare(
+        "dynamic_heat_balance",
+        ConfigValue(
+            default=True,


Two comments here:

Do we want to allow this to take a default value based on the flowsheet, in the same way the dynamic flag does? In this case, we should make the default useDefault and the domain DefaultBool (see https://github.com/IDAES/idaes-pse/blob/main/idaes/core/unit_model.py).

I think it would be helpful to add validation of this config argument in the code (and it would be necessary if we do suggestion 1). E.g. we should check for dynamic_heat_balance=True in flowsheets where dynamic=False and raise an exception of our own to inform users what happened. Otherwise, they will eventually see a DAE_Error, but that is likely not going to help them understand what they did wrong and how to fix it.

Agreed. useDefault makes sense here. Regarding point 2., I found this: https://github.com/IDAES/idaes-pse/blob/main/idaes/core/util/exceptions.py, which seems more appropriate than DAE_Error

…acitance, added useDefault to dynamic_heat_balance

andrewlee94

Looks good to me.

rustygentile added 5 commits February 13, 2022 01:03

added initial version of lumped capacitance heat exchanger (IDAES#673)

b74da1c

Merge branch 'main' into 673-heat-exchanger

d966940

updated cross references in heat exhanger lumped capaticance docs, up…

95f646b

…dated failing regression test

fixed sample code in heat exchanger lumped capacitance docs

84bda81

updated heat exchanger lumped capacitance docs example to use plot_gr…

a28d871

…id_dynamic() and save to file

andrewlee94 added Priority:Normal Normal Priority Issue or PR unit models Issues dealing with the unit model libraries user request Requests from external users labels Feb 18, 2022

andrewlee94 requested review from andrewlee94 and eslickj February 18, 2022 15:00

andrewlee94 reviewed Feb 18, 2022

View reviewed changes

eslickj reviewed Feb 18, 2022

View reviewed changes

idaes/generic_models/unit_models/heat_exchanger_lc.py Show resolved Hide resolved

rustygentile added 2 commits February 24, 2022 16:51

addressing PR comments: added xfail to test_units, added author credi…

d26f827

…t to docs page, removed unneeded CONFIG line in heat_exchanger_lc

added an explicit dynamic_heat_balance option to heat exchanger lumpe…

4fc2020

…d capacitance config

andrewlee94 reviewed Mar 4, 2022

View reviewed changes

added error messages for invalid configs to heat exchanger lumped cap…

02e5acf

…acitance, added useDefault to dynamic_heat_balance

andrewlee94 approved these changes Mar 22, 2022

View reviewed changes

eslickj approved these changes Mar 22, 2022

View reviewed changes

lbianchi-lbl merged commit cd2c97c into IDAES:main Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wall temperature and heat accumulation terms to the 0D heat exchanger #714

Add wall temperature and heat accumulation terms to the 0D heat exchanger #714

rustygentile commented Feb 17, 2022

andrewlee94 left a comment

andrewlee94 Feb 18, 2022

rustygentile Feb 24, 2022

andrewlee94 Feb 25, 2022

rustygentile Feb 27, 2022

andrewlee94 Feb 28, 2022

rustygentile Feb 28, 2022

andrewlee94 Feb 28, 2022

rustygentile Feb 28, 2022

andrewlee94 Feb 28, 2022

rustygentile Mar 3, 2022

codecov bot commented Feb 18, 2022 •

edited

Loading

andrewlee94 left a comment

andrewlee94 Mar 4, 2022

rustygentile Mar 9, 2022

andrewlee94 Mar 9, 2022

andrewlee94 Mar 4, 2022

rustygentile Mar 9, 2022

andrewlee94 left a comment


		def initialize(self, args, *kwargs):

		if self.flowsheet().config.dynamic:

Add wall temperature and heat accumulation terms to the 0D heat exchanger #714

Add wall temperature and heat accumulation terms to the 0D heat exchanger #714

Conversation

rustygentile commented Feb 17, 2022

Fixes

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

andrewlee94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

andrewlee94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewlee94 left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2022 •

edited

Loading