Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mksurfdata_esmf: Refactor mksoiltex #2737

Open
wants to merge 3 commits into
base: b4b-dev
Choose a base branch
from

Conversation

samsrabin
Copy link
Collaborator

@samsrabin samsrabin commented Aug 30, 2024

Description of changes

Refactors mksoiltex() subroutine to make it easier to understand and avoid duplicated code.

Specific notes

Contributors other than yourself, if any: None

CTSM Issues Fixed (include github issue #): None

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:

  • Builds without error
  • Runs without error
  • Outputs are bit-for-bit identical with previous version (organic_o option 1)
  • Outputs are bit-for-bit identical with previous version (disabled organic_o option 2) Removing that code

Other remaining tasks

  • Rename is_neg_4 to be more meaningful. I think it indicates sand dunes, but is that really true for ALL variables or just PCT_SAND?
  • Can block at TODO: Remove as unnecessary? be removed?
  • Remove organic_o option 2

@samsrabin samsrabin added code health improving internal code structure to make easier to maintain (sustainability) support tools only bfb bit-for-bit labels Aug 30, 2024
@samsrabin samsrabin added the priority: low Background task that doesn't need to be done right away. label Aug 30, 2024
@samsrabin
Copy link
Collaborator Author

Output surface dataset at 1.9x2.5 resolution as of 9a2759f is bit-for-bit identical with version from ctsm5.2.027. However, my implementation of organic_o option 2—which is hard-coded off, as before—produces differences when forced on. I wonder if it would make sense at this point to just remove that code?

@samsrabin samsrabin added the PR status: awaiting review Work on this PR is paused while waiting for review. label Sep 4, 2024
@slevis-lmwg
Copy link
Contributor

@samsrabin I think it's ok to remove "option 2" at this point. There's record of it if we ever decide to resuscitate it.

Also, is this PR affected by the latest updates that occurred here?

@samsrabin
Copy link
Collaborator Author

Thankfully no, this touches a different part of the code.

@samsrabin samsrabin removed the PR status: awaiting review Work on this PR is paused while waiting for review. label Sep 25, 2024
@samsrabin samsrabin added test: mksurfdata Test mksurfdata_esmf before merging and removed support tools only labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit code health improving internal code structure to make easier to maintain (sustainability) priority: low Background task that doesn't need to be done right away. test: mksurfdata Test mksurfdata_esmf before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants