Repository navigation
Conversation
This was a 1x1 copy from IPW and is no filtered to what is used within SMRF. Also added the Stefan-Boltzmann constant.
… code There was duplicated code for the math to calculate the saturation vapor pressure. One version for Python and one for the C extension. This combines the math to be only within C. It also cleans up the `sati` and `satw` function and creates a "neutral" endpoint to move the decision for ice vs water into a new method. Also adds some references to the methods itself.
There was a problem hiding this comment.
Pull request overview
This PR refactors saturation vapor pressure computation in SMRF by introducing a unified saturation_vapor_pressure interface (C + Python), replacing direct usage patterns around satw/sati, and cleaning up related C headers/wrappers. This supports consistent physics behavior across the C kernels and the Python API while reducing duplicated formula logic.
Changes:
- Added a C-level
saturation_vapor_pressure(double *temperature)wrapper and updated C call sites to use it. - Updated the Python vapor pressure API to call into a new Cython wrapper (
svp_for_temperatures) for array computations. - Removed/centralized some constants/macros in
envphys.hand added a new unit test for vapor pressure.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| smrf/tests/envphys/test_vapor_pressure.py | Adds a unit test for saturation_vapor_pressure. |
| smrf/envphys/vapor_pressure.py | Replaces pure-Python satw/sati with a unified wrapper calling into Cython/C. |
| smrf/envphys/thermal/topotherm.py | Switches thermal calculations to use saturation_vapor_pressure. |
| smrf/envphys/core/topotherm.c | Adds C wrapper saturation_vapor_pressure and refactors satw/sati implementations. |
| smrf/envphys/core/envphys.h | Cleans up constants/macros and introduces LOG_10 and STEF_BOLTZ. |
| smrf/envphys/core/envphys_c.pyx | Adds svp_for_temperatures Cython wrapper and updates extern declarations. |
| smrf/envphys/core/envphys_c.h | Exposes saturation_vapor_pressure in the C API header. |
| smrf/envphys/core/dewpt.c | Updates dew point routines to use the unified saturation vapor pressure wrapper. |
Turns out that having it as a constants will make tests flaky once the shared mock is changed or updated as it is the case with the albedo tests.
Previously untested.
Missed this call to sati and was indentified by @copilots review.
This uses the same file as pysnobal to create a consistent code style. Also moved one more constant declaration to envphys.h
Rename satvp to svp_for_celsius since that is all the method does.
Most noteworthy is to use "value != value" as a shortcut to check for NaNs in C.
… functions ABS and EPSILON are defined as part of standard C in tgmath.h and float.h
Update to use numpy memoryviews for future compatibility. Also add/update docstrings.
jomey
added
maintenance
Those are no longer called individually and available via the saturation_vapor_pressure API
C offers a log10 method that simplifies the math for sati and satw calculations.
Clarify units for temperature
Cleans, builds, and then installs the package in editable mode
Explicitly add these targets before tests and install
Transfer from pysnobal, where a similar commit was done recently. Copilot explanation: Defining NPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION tells NumPy's headers: "compile against only the stable post-1.7 API." This is the same API surface that NumPy 2.x retains — the symbols that were removed in 2.0 are a strict subset of what was already deprecated before 1.7. Placing the macro in setup.py's Extension definition means it is applied uniformly to every .c file compiled in that extension (including the libsnobal C sources), not just the Cython-generated one.
Contributor
|
Ready to pull the trigger...happy to see it finally worked! |
jomey
commented
Sep 17, 2026
| ) | ||
| "extra_link_args": ["-fopenmp"], | ||
| "include_dirs": [numpy.get_include()], | ||
| "define_macros": [("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")], |
Member
Author
There was a problem hiding this comment.
This removes the numpy compile warning
Member
Author
Ready to review including the corresponding AWSM PR. Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduction of a unified
saturation_vapor_pressurefunction, which replaces direct calls tosatwandsatiwith a single interface in both C and Python code. Additionally, several unused or redundant constants and macros are removed fromenvphys.h, and the code style is cleaned up for consistency.The most important changes are:
Saturation Vapor Pressure Refactor:
saturation_vapor_pressure(double *temperature)function in C (topotherm.c/envphys_c.h) that selects the appropriate formula (over ice or water) based on temperature, replacing direct calls tosatwandsatithroughout the codebase.svp_for_temperaturesto efficiently apply the new saturation vapor pressure function to arrays.Code Cleanup:
envphys.hTests
This will make the AWSM tests fail since we are changing how floating point variables are used and replace a Python with a C calculation. All the changes to the test files are in the range of floating point number differences.
The corresponding PR: iSnobal/awsm/pull/40
Food for thought
This could also be a first template on how to approach the
tk less than zeroerror in pysnobal, by making less of a wild goose chase where and howsatiis called. Ideally we would not have the copied code from IPW in SMRF, but then you wouldn't be able to install SMRF without pysnobal. The latter is something I would happily introduce as a library dependency with SMRF since pysnobal is a quick and lightweight install. Thoughts?