Python scripts: Require user to specify lon format when ambiguous #3024
Conversation
This file contains 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
changed the title
7 tasks
|
* Require that user specify whether longitude is in [-180, 180] format (i.e., centered around Prime Meridian) or [0, 360] format (i.e., centered around International Date Line). * Specified as --lon-type, either 180 or 360 (respectively) * Resolves ESCOMP#2017 * Resolves ESCOMP#3001
samsrabin
force-pushed
the
fix-python-tools-longitude-format
branch
from
dd68889 to
6b29b53
Compare
samsrabin
added
bug
github-project-automation
bot
moved this to Ready to start (or start again)
in CTSM: Upcoming tags
samsrabin
moved this from Ready to start (or start again)
to In progress - master/b4b-dev
in CTSM: Upcoming tags
samsrabin
commented
Apr 3, 2025
samsrabin
commented
Apr 3, 2025
Also improve an error message.
samsrabin
force-pushed
the
fix-python-tools-longitude-format
branch
from
a777427 to
27d45c9
Compare
samsrabin
force-pushed
the
fix-python-tools-longitude-format
branch
from
879ddd8 to
7ca9ad0
Compare
There was a problem hiding this comment.
Copilot reviewed 20 out of 25 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- .git-blame-ignore-revs: Language not supported
- python/ctsm/test/testinputs/modify_fsurdat_1x1mexicocity.cfg: Language not supported
- python/ctsm/test/testinputs/modify_fsurdat_opt_sections.cfg: Language not supported
- python/ctsm/test/testinputs/modify_fsurdat_short.cfg: Language not supported
- python/ctsm/test/testinputs/modify_fsurdat_short_nofiles.cfg: Language not supported
Comments suppressed due to low confidence (2)
python/ctsm/test/test_sys_mesh_modifier.py:60
- The variable _lon_type is set to None in the test setup but never given a proper default value. Consider initializing it with an appropriate default (e.g., 360) to ensure consistent behavior in tests.
self._lon_type = None
python/ctsm/subset_data.py:440
- The condition intended to check for the existence of the config file uses surf_year as a proxy, leading to a misleading error message (“Entered default config file does not exist”). Clarify the condition by directly checking the existence of the config file and update the corresponding error message accordingly.
if args.surf_year != 1850 and args.surf_year != 2000:
samsrabin
commented
Apr 5, 2025
| @@ -119,6 +137,7 @@ def get_parser(): | |||
| type=plon_type, | |||
| default=287.8, | |||
There was a problem hiding this comment.
- Add "must be unambiguous" note here.
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.
Description of changes
At the moment, our Python tools are dangerous in that they can allow the user to enter longitude in the [-180, 180] format when the tools actually assume the [0, 360] format. This PR fixes that by, in ambiguous cases, requiring the user to specify the format they're using with
--lon-typeeither 180 or 360.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? Yes; see ESCOMP/ctsm-docs#9.
Testing performed, if any:
Remaining:
clm_pymodstest suite