Allow upwinding in 1D Heat exchanger by dallan-keylogic · Pull Request #1383 · IDAES/idaes-pse

dallan-keylogic · 2024-03-22T16:45:59Z

Summary/Motivation:

Breaks off the changes to the HeatExchanger1D from #1382 into their own PR

Changes proposed in this PR:

Allows upwind difference schemes to be used in the HeatExchanger1D
Adds warnings about energy conservation when different finite difference schemes are used on either side of the heat exchanger.

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.

codecov · 2024-03-22T17:36:09Z

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.63%. Comparing base (8948c6c) to head (1217e87).

Files	Patch %	Lines
idaes/models/unit_models/heat_exchanger_1D.py	93.54%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1383   +/-   ##
=======================================
  Coverage   77.62%   77.63%           
=======================================
  Files         391      391           
  Lines       64375    64380    +5     
  Branches    14257    14261    +4     
=======================================
+ Hits        49973    49979    +6     
- Misses      11830    11835    +5     
+ Partials     2572     2566    -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

idaes/models/unit_models/tests/test_heat_exchanger_1D.py

bpaul4

Everything looks good, @dallan-keylogic.

andrewlee94

A lot of comments ,but most are really minor.

Regarding the level of logger messages, I can see why you put them at "warning" level, but I think we should reserve that for cases where something is going wrong, and not places where we are using default settings.

andrewlee94 · 2024-03-26T14:24:09Z

idaes/models/unit_models/heat_exchanger_1D.py

-            set_direction_hot = FlowDirection.forward
-            set_direction_cold = FlowDirection.forward
+        if self.config.hot_side.transformation_method is useDefault:
+            _log.warning(


Should this be a warning level message? I am of two minds, but I think this would be better as info.

This was originally a warning-level message before my proposed revisions. I'd be happy to downgrade it to "Caution" or "Info".

Maybe make it a caution then - it is definitely not a warning but probably warrants more than info.

andrewlee94 · 2024-03-26T14:24:18Z

idaes/models/unit_models/heat_exchanger_1D.py

+            )
+            self.config.hot_side.transformation_method = "dae.finite_difference"
+        if self.config.cold_side.transformation_method is useDefault:
+            _log.warning(


idaes/models/unit_models/heat_exchanger_1D.py

andrewlee94 · 2024-03-26T14:27:35Z

idaes/models/unit_models/heat_exchanger_1D.py

+                self.config.hot_side.transformation_scheme is useDefault
+                or self.config.cold_side.transformation_scheme is useDefault
+            ):
+                raise ConfigurationError(


Minor comment, but should we have a default here? I vaguely recall that one of the collocation schemes had a bug too.

Perhaps. Originally I wanted the user to assume full responsibility for using collocation, but since we need it for energy conservation in upwind schemes I might make "LAGRANGE-RADAU" the default. I should test it out on "LAGRANGE-LEGENDRE" as well, because that's the one that potentially has a bug in it; it creates additional continuity equations about the values of the function between finite elements, and I can easily see that it fails to do so correctly when flow is moving from 1 to 0.

My vague recollection is the it was Lagrange-Legendre that had the possible bug. If so - we should make Lagrange-Radau the default.

Is there an issue for this bug with Lagrange-Legendre?

@blnicho @andrewlee94 It appears to work just fine for the HeatExchanger1D in counter-current mode, and, furthermore, both methods return the same answers for outlet temperatures.

idaes/models/unit_models/heat_exchanger_1D.py

andrewlee94 · 2024-03-26T14:35:22Z

idaes/models/unit_models/heat_exchanger_1D.py

+        f"For {side}, a {bad_scheme} scheme was chosen to discretize the length domain. "
+        f"However, this scheme is not an upwind scheme for {flow_config} flow, and "
+        f"as a result may run into numerical stability issues. To avoid this, "
+        f"use a {good_scheme} scheme (which may result into energy conservation issues "


Typo: into -> in

idaes/models/unit_models/tests/test_heat_exchanger_1D.py

codecov-commenter · 2024-05-02T19:16:31Z

Codecov Report

Attention: Patch coverage is 94.73684% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.89%. Comparing base (5d2f94c) to head (471cd42).

Files	Patch %	Lines
idaes/models/unit_models/heat_exchanger_1D.py	93.93%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1383   +/-   ##
=======================================
  Coverage   77.89%   77.89%           
=======================================
  Files         394      394           
  Lines       65053    65063   +10     
  Branches    14383    14387    +4     
=======================================
+ Hits        50670    50679    +9     
- Misses      11793    11798    +5     
+ Partials     2590     2586    -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewlee94 · 2024-05-03T13:57:40Z

idaes/logger.py

 CRITICAL = logging.CRITICAL  # 50
 ERROR = logging.ERROR  # 40
 WARNING = logging.WARNING  # 30
+CAUTION = 25


I originally wondered if it would just be better to use INFO_LOW for the "cautions" rather than add additional logger levels, however the naming of the INFO levels is somewhat counter-intuitive (named by amount of output rather than importance). In that sense, I think I am actually inclined to keep CAUTION as being more intuitive.

andrewlee94 · 2024-05-03T17:59:41Z

@dallan-keylogic It looks like some tests are failing due to numerical diagnostics checks (possibly due to merging the near parallel constraint check into the main diagnostics toolbox).

dallan-keylogic · 2024-05-10T21:20:10Z

@lbianchi-lbl @ksbeattie Punting this was unsuccessful. Codecov demands its pound of flesh.

lbianchi-lbl · 2024-05-10T22:19:44Z

@lbianchi-lbl @ksbeattie Punting this was unsuccessful. Codecov demands its pound of flesh.

It looks like the jobs on our side (i.e. the only thing we have control over) are passing:

The reason why the required checks don't show up is on Codecov's side. Unfortunately, there's not much we can do other than maybe trying to re-run the CI (using git commit --allow-empty -m "Trigger CI" && git push ain't the prettiest, but it should work).

dallan-keylogic added 6 commits March 22, 2024 09:54

changes

7485d7a

Test config validation

b26f9d3

add back original countercurrent test

5a3d18a

Run Black

2b5fcf3

merge

a59e23c

Add back tests lost in merge

beffd8a

dallan-keylogic requested review from andrewlee94 and bpaul4 as code owners March 22, 2024 16:46

dallan-keylogic changed the title ~~Heat exchanger 1d upwind~~ Allow upwinding in 1D Heat exchanger Mar 22, 2024

bpaul4 reviewed Mar 25, 2024

View reviewed changes

idaes/models/unit_models/tests/test_heat_exchanger_1D.py Show resolved Hide resolved

Comment on logic of test

0678e81

dallan-keylogic requested review from ksbeattie and lbianchi-lbl as code owners March 25, 2024 15:46

dallan-keylogic added 2 commits March 25, 2024 11:47

somehow config.yaml returned

0d578d5

actually commit comment

1217e87

bpaul4 approved these changes Mar 25, 2024

View reviewed changes

andrewlee94 reviewed Mar 26, 2024

View reviewed changes

ksbeattie requested a review from jinliangma March 26, 2024 21:59

ksbeattie added the Priority:Normal Normal Priority Issue or PR label Mar 28, 2024

ksbeattie assigned dallan-keylogic Mar 28, 2024

dallan-keylogic added 5 commits May 2, 2024 11:34

Merge branch 'main' into heat_exchanger_1d_upwind

a89b260

caution

3ac2507

divide tests

20de061

black

e4c7ecf

test block triangularization for collocation

69c9954

dallan-keylogic requested a review from andrewlee94 May 3, 2024 13:03

andrewlee94 approved these changes May 3, 2024

View reviewed changes

andrewlee94 enabled auto-merge (squash) May 3, 2024 13:58

Merge branch 'main' into heat_exchanger_1d_upwind

89f2af8

andrewlee94 disabled auto-merge May 3, 2024 17:57

dallan-keylogic and others added 3 commits May 8, 2024 13:48

Merge branch 'main' into heat_exchanger_1d_upwind

21a65c7

scale model

44756c9

Merge branch 'main' into heat_exchanger_1d_upwind

471cd42

Merge branch 'main' into heat_exchanger_1d_upwind

03aced0

andrewlee94 merged commit f0c60c4 into IDAES:main May 16, 2024

Comments

Conversation

dallan-keylogic commented Mar 22, 2024

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

Uh oh!

codecov bot commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

bpaul4 left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlee94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewlee94 commented May 3, 2024

Uh oh!

dallan-keylogic commented May 10, 2024

Uh oh!

lbianchi-lbl commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Mar 22, 2024 •

edited

Loading

codecov-commenter commented May 2, 2024 •

edited

Loading

lbianchi-lbl commented May 10, 2024 •

edited

Loading