Crossflow hx #1382
Crossflow hx #1382
Conversation
dallan-keylogic
requested review from
bpaul4,
MAZamarripa and
andrewlee94
as code owners
|
The test failures are from what were supposed to be minor changes to the HeatExchanger1D model. Apparently having opposite discretization methods on both sides causes problems with the energy balance. This is a function of coarse discretization---when I switched to collocation the problem disappeared. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1382 +/- ##
==========================================
+ Coverage 77.62% 77.69% +0.07%
==========================================
Files 391 394 +3
Lines 64375 65030 +655
Branches 14257 14384 +127
==========================================
+ Hits 49973 50527 +554
- Misses 11830 11895 +65
- Partials 2572 2608 +36 ☔ View full report in Codecov by Sentry. |
idaes/models_extra/power_generation/unit_models/heat_exchanger_common.py
Show resolved
Hide resolved
idaes/models_extra/power_generation/unit_models/heat_exchanger_common.py
Outdated
Show resolved
Hide resolved
| =========================== ============ ================================================================================= | ||
|
|
||
|
|
||
| Initialization |
There was a problem hiding this comment.
I would suggest moving the text here to the Initializer doc string (if it isn't there already) and use autodocs here. That way the user gets to see the arguments for the initializer and a hint to the name to import.
|
|
||
| @pytest.mark.integration | ||
| def test_units(model_no_dP): | ||
| assert_units_consistent(model_no_dP.fs.heat_exchanger) |
There was a problem hiding this comment.
Would it be possible to replace this with a model diagnostics check?
dt = DiagnosticsToolbox(m)
dt.assert_no_structural_issues()
This includes unit consistency, as well as all the other structural checks. There is an equivalent method for numerical checks too.
This is not critical for contrib model, but is good if we can do it.
| # FIXME estimate from parameters | ||
| if blk.config.has_holdup: | ||
| s_U_holdup = gsf(blk.heat_holdup[t, z]) | ||
| cst(blk.heat_holdup_eqn[t, z], s_U_holdup) |
There was a problem hiding this comment.
Not necessary to do unless you want to (more just suggestions for future developements), but it would be nice to have some unit tests for these. They will get covered by the unit models, but unit testing these might help narrow down where failures are occurring if something does go wrong. I also see a lot of opportunities for breaking out smaller private functions that make up the main public methods for testing here.
There was a problem hiding this comment.
Unit tests for scaling factors? Probably better to have some assertion about no numerical issues from the diagnostics toolbox.
There was a problem hiding this comment.
You really want both - the purpose of the unit test is not to check the scaling factors themselves but that the code runs smoothly (i.e. to help you narrow down where the issues is occurring rather than having to work throguh the entire unit-level method to find where it breaks). You do need a separate set of tests to make sure that the scaling factors do improve the condition of the model (for limited test cases at least).
| model.control_volume.pressure.fix() | ||
|
|
||
| model.control_volume.length.fix() | ||
| assert degrees_of_freedom(model.control_volume) == 0 |
There was a problem hiding this comment.
You probably should not have an assert here, because if this fails it won't give the user a useful error message. The base class already does an initial DoF check, so you can probably get away without this here.
| with idaeslog.solver_log(solve_log, idaeslog.DEBUG) as slc: | ||
| res = solver_obj.solve(model.control_volume, tee=slc.tee) | ||
|
|
||
| assert_optimal_termination(res) |
There was a problem hiding this comment.
Similarly, I think you need a try/except here to emit a useful error message.
| model.control_volume.pressure.unfix() | ||
| model.control_volume.pressure[:, 0].fix() | ||
|
|
||
| assert degrees_of_freedom(model) == 0 |
There was a problem hiding this comment.
As above, and a few lines below too.
|
|
||
| CONFIG = UnitModelBlockData.CONFIG() | ||
| CONFIG.declare( | ||
| "has_fluid_holdup", |
There was a problem hiding this comment.
Is this distinct from the has_holdup that UnitModelBlockData.CONFIG already has? If so, a little more explanation of the difference might be needed. I suspect you are trying to separate energy holdup form fluid holdup.
There was a problem hiding this comment.
Yes, it is. It's a stub for potentially having mass and energy holdup in the fluid (as opposed to the energy holdup in the walls). However, due to concerns about pressure waves, its only valid value is False for the time being.
| ), | ||
| ) | ||
|
|
||
| CONFIG = CONFIG |
There was a problem hiding this comment.
This seems to be unnecessary.
| Reference, | ||
| units as pyunits, | ||
| ) | ||
| import pyomo.opt |
There was a problem hiding this comment.
Do you need the blanket import for pyomo.opt?
| # Important to do before initializing property packages in | ||
| # case it is implemented as Var-Constraint pair instead of | ||
| # an Expression | ||
| value(hot_side.properties[t0, 0].cp_mol) |
There was a problem hiding this comment.
Minor point, but you don't even need the value here - it is enough to just call hot_side.properties[t0, 0].cp_mol
There was a problem hiding this comment.
I took those out, but then Pylint complained the statements were pointless, so I put them back in.
There was a problem hiding this comment.
Went back and took the values out while disabling Pylint warnings for them because there's no guarantee the evaluation won't cause some error before the model is initialized.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1382 +/- ##
==========================================
+ Coverage 77.63% 77.69% +0.06%
==========================================
Files 391 394 +3
Lines 64391 65033 +642
Branches 14264 14380 +116
==========================================
+ Hits 49989 50530 +541
- Misses 11830 11903 +73
- Partials 2572 2600 +28 ☔ View full report in Codecov by Sentry. |
Summary/Motivation:
Adds models for crossflow heat exchanger and heater used in SOC dynamic flowsheet to models_extra.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: