Fix Longitude comparison error for regional subset_data #3100
Conversation
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
samsrabin
added
bug
samsrabin
force-pushed
the
subset-data-lon-3093
branch
from
eddd7d9 to
443f0ac
Compare
samsrabin
force-pushed
the
subset-data-lon-3093
branch
from
443f0ac to
8797c44
Compare
samsrabin
changed the title
samsrabin
added
next
samsrabin
commented
May 6, 2025
ekluzek
requested changes
May 6, 2025
Collaborator
ekluzek
left a comment
ekluzek
left a comment
There was a problem hiding this comment.
This is great work. And I think it's a great example of refactoring to improve the code, rather than putting up with brittle code. You add unit and system testing that gives us much more confidence that subset data is working correctly and will continue working.
I do have a bunch of suggestions. As always many I'm flexible on. Some I think will be helpful to do. Some I may be wrong on. So please reach out if you disagree.
Again though, this is a great improvement to our testing for subset_data, and hopefully will catch most future bugs from happening. It gives me so much more confidence in our testing of the important tool of subset_data.
Member
Author
|
Thanks, @ekluzek! |
ekluzek
removed
the
PR status: awaiting review
This ensures that GitHub format workflow fails if only Pylint has errors (i.e., Black passes).
Merge b4bdev 20250509 Updates to the b4b-dev branch since its last merge to master (PRs ESCOMP#3091 ESCOMP#3092), as shown by git log: - Merge pull request Update docs infrastructure ESCOMP#2809 from samsrabin/update-docs-builder-2 Update docs infrastructure - Merge pull request Update externals to cesm3_0_alpha06c ESCOMP#3106 from ekluzek/update_to_alpha06c Update externals to cesm3_0_alpha06c - Merge pull request User control over snow thermal conductivity scheme over glaciers ESCOMP#3072 from wwieder/JordanGlacier User control over snow thermal conductivity scheme over glaciers
samsrabin
added
PR status: awaiting review
ekluzek
approved these changes
May 15, 2025
ekluzek
removed
the
PR status: awaiting review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description of changes
RegionalCaseno longer (necessarily) fails with an error about comparisons not being supported between float and Longitude. It now tries to convert input file longitude values to match_lon_typeof--lon1and--lon2. This is tested with new system tests ofsubset_data.This capability required several supporting changes and tests thereof:
Longitudetype work if initialized with lists or arrays.subset_datano longer fails on missing but unused config file sections. E.g., if you didn't say--create-landuse, it won't try to read anything from the config file'slandusesection.Note that
RegionalCasewill still fail if the input file's longitude values are (a) of an ambiguous type or (b) of a different type than--lon1and--lon2. It would be good to resolve this, but perhaps that should be saved for a future PR.Specific notes
Contributors other than yourself, if any: None
CTSM Issues Fixed:
Are answers expected to change (and if so in what way)? No
Any User Interface Changes (namelist or namelist defaults changes)? No
Does this create a need to change or add documentation? Did you do so? No
Testing performed, if any:
As of 3fa2980:
clm_pymodssuite compared to ctsm5.3.043 (/glade/derecho/scratch/samrabin/tests_0515-105341de/)