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

Fix bug introduced by 443a2fd #2351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

msaitta-mpr
Copy link
Contributor

Description of the Change

Commit 443a2fd added some extra checks for convergence in the Helmholtz PHSU flash routines. However, its checking was more aggressive than the original routine. This caused the routine to raise an exception on several values that had previously been deemed acceptable. This trickled down to cause the TTSE tables to be filled with empty values, making them less robust.

This commit allows for a solution to be acceptable if the change is small between iterations as was originally intended.

Benefits

The docs now build properly.

Possible Drawbacks

None known. However we should ensure that the tests pass acceptably.

Verification Process

I manually ran Web/scripts/coolprop.tabular.speed.py with the updated code and with the current master branch. Using master causes a crash - this was what was causing the docs the fail to build in the github pipeline. Running with this new commit causes coolprop.tabular.speed.py to run to completion.

Applicable Issues

Closes #2349

Commit 443a2fd addedsome extra checks for convergence in the Helmholtz
PHSU flash routines. However, its checking was more aggressive than the
original routine. This caused the routine to raise an exception on
several values that had previously been deemed acceptable. This trickled
down to cause the TTSE tables to be filled with empty values, making
them less robust.

This commit allows for a solution to be acceptable if the change is
small between iterations as was originally intended.
@jowr
Copy link
Member

jowr commented Feb 16, 2024

Thank you very much - I can review it next week!

@msaitta-mpr
Copy link
Contributor Author

@jowr - Have you had a chance to review the PR yet?

@ibell
Copy link
Contributor

ibell commented Mar 22, 2024

I think this seems reasonable. Shall I merge?

@msaitta-mpr
Copy link
Contributor Author

I see no reason not to, but I defer to @ibell and @jowr on such matters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The docs are broken since md-January
3 participants