Skip to content

Conversation

@glwagner
Copy link
Member

This PR fixes a bug in the saturation adjustment solver that only gets hit under certain conditions: when the secant iteration accesses unsaturated states during the course of finding a saturated state. Before this PR, we assume that during adjustment the state was saturated. However, this assumption does not hold if "intermediate" (eg test) states that are accessed during the iteration are not saturated. The result of the bug is a spurious change in the total moisture fraction. It is fixed in this PR.

This PR fixes a bug in the saturation adjustment solver that only gets hit under certain conditions: when the secant iteration accesses unsaturated states during the course of finding a saturated state. Before this PR, we assume that during adjustment the state was saturated. However, this assumption does not hold if "intermediate" (eg test) states that are accessed during the iteration are not saturated. The result of the bug is a spurious change in the total moisture fraction. It is fixed in this PR.
@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@glwagner glwagner requested a review from navidcy November 10, 2025 05:30
@glwagner
Copy link
Member Author

merge when ready

@giordano
Copy link
Collaborator

Should this have a regression test?

@glwagner
Copy link
Member Author

@giordano yes! Even more important is the atmospheremodel. But the boussinesq solver represents our work over the past few months (foundational work) and it would be nice to preserve.

@glwagner
Copy link
Member Author

i think we should build regression tests from the canonical cases (eg bomex, and there are another dozen that we will implement)

@navidcy
Copy link
Member

navidcy commented Nov 12, 2025

@glwagner, resolve conflicts?

@glwagner
Copy link
Member Author

another route for this PR is to use the main adjustment solver, which now supports potential temperature state. so we don't have to merge this necessarily (i'm fine either way)

Copy link
Member

@navidcy navidcy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glwagner, your call whether to merge or not!

@glwagner
Copy link
Member Author

better to work than not I think!

@glwagner glwagner merged commit 506bc3f into main Nov 12, 2025
11 checks passed
@glwagner glwagner deleted the glwagner-patch-1 branch November 12, 2025 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants