Conversation
Re-run the topo.nc file sky view, terrain factor, and slope using the updated sky view method from TopoCalc. All the changed gold files had the sky view factor as an argument. Difference plots are provided with the PR.
The calculations used sky view factor as part of the terrain correction which updated in the most recent TopoCalc library.
Add a class variable that prevents the deletion of output files once a comparison with a gold file fails. This enables troubleshooting of differences.
Add a target that stops after the first test failure.
There was a problem hiding this comment.
Pull request overview
Updates SMRF’s test suite and related tooling to reflect TopoCalc’s updated sky view factor behavior (self-shading), adds additional topo “warning system” tests, and introduces a fast-fail test runner option while providing notebooks to help visualize NetCDF diffs.
Changes:
- Update expected values in solar/toporad tests to match new TopoCalc sky view factor outputs.
- Add topo tests that regenerate missing sky view factor outputs and compare against gold topo values within tolerance.
- Add a fast-fail unittest target and preserve outputs on NetCDF comparison failures.
Reviewed changes
Copilot reviewed 7 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| smrf/tests/smrf_test_case.py | Adds “preserve output on NetCDF compare failure” behavior and refactors comparisons. |
| smrf/tests/envphys/solar/test_toporad.py | Updates assertions for beam/diffuse min/max values after SVF changes. |
| smrf/tests/data/test_topo.py | Adds tests for regenerating missing SVF outputs and comparing against topo gold. |
| smrf/tests/basins/Topo-Compare.ipynb | Adds a notebook to compare topo NetCDF variables visually. |
| smrf/tests/basins/Gold-NetCDF-Compare.ipynb | Adds a notebook to compare new outputs vs gold NetCDF files visually. |
| smrf/data/load_topo.py | Refactors string/style and updates SVF variable creation/compression usage. |
| Makefile | Adds tests_fast_fail target (unittest -f). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jomey
commented
Mar 13, 2026
Some suggestions are around test logic. The most significant suggestion is to reset the read values from a topo file before assigning it to an instance variable. This ensure that wrong values are not set for a variable.
Previously removed, but shouldn't as it is still executed with the init
brentwilder
approved these changes
Mar 13, 2026
Contributor
brentwilder
left a comment
brentwilder
left a comment
There was a problem hiding this comment.
This looks great @jomey , thanks for going through and updating all of the tests! I also appreciate the notebook for next time for convenience
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.
Second part of the test suite update after including self-shading of slopes in TopoCalc (iSnobal/topocalc#12)
Outside of the gold file NetCDF updates, it also adds tests for the topo to compare the terrain variables and how a "fresh" calculation would potentially change things. This aim is to have a little warning system when something would change significantly.
It also adds a convenience feature where tests can be run and stop at the first failure and preserve the simulated output that did not compare within the tolerance with the gold files.
NetCDF changes
Most of the changes will show the pattern from the sky view changes, where that was an input to calculate radiation.
Lakes Topo
Thermal (theoretical)
Thermal HRRR
Thermal HRRR with vegetation correction
Net Solar (theoretical SW)
Net Solar HRRR
Net Solar HRRR with vegetation correction