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

API 36.1.0 merge #1255

Merged
merged 30 commits into from
Oct 4, 2024
Merged

API 36.1.0 merge #1255

merged 30 commits into from
Oct 4, 2024

Conversation

glemieux
Copy link
Contributor

@glemieux glemieux commented Oct 2, 2024

Integrating this PR will merge in #1136 ad #1236. To be associated with ESCOMP/CTSM#2700.

Description:

This is a pull request to coordinate the update of multiple parameter file updates at the same time. Whereas #1136 and #1236 were tested separately with their own special parameter file updates, this PR will combined the parameter file updates into the single default source. Part of the reason for setting things up this way is to provide more clarity in the testing, but also to capture the workflow necessary to update the default parameter file as it requires a sequential calls of UpdateParamAPI followed by BatchPatchParams tool on the #1136 and #1236 patch file updates respectively.

Collaborators:

@JessicaNeedham @jenniferholm @rgknox @XiulinGao

Expectation of Answer Changes:

Yes, answers are expected to change due to the parameter file updates. See the respective pull requests for more detail.

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

  • The in-code documentation has been updated with descriptive comments
  • The documentation has been assessed to determine if updates are necessary

Integrator

  • FATES PASS/FAIL regression tests were run
  • Evaluation of test results for answer changes was performed and results provided

Documentation

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

JessicaNeedham and others added 26 commits October 2, 2024 15:20
Per discussions, we decided to not make this a separate parameter file
and to roll it into the default.
The files have been appended with numbers to indicated the sequence in
which they should be applied to the archived parameter file.
@glemieux glemieux requested a review from rgknox October 2, 2024 22:58
This is the result of following the workflow to combine patch file
updates from NGEET#1136 and NGEET#1236
glemieux added a commit to rgknox/ctsm that referenced this pull request Oct 2, 2024
This update incorporates multiple parameter file updates per NGEET/fates#1255
@glemieux
Copy link
Contributor Author

glemieux commented Oct 2, 2024

  • Update the default parameter file with the correct names for the new pfts

I think this requires a more up-to-date version of numpy than I'm currently using (numpy==1.24.4) as modify_fates_paramfile.py returns a warning and skips the update:

../tools/modify_fates_paramfile.py --fin /tmp/tmp.hdMg69N3Mi --fout /tmp/tmp.hdMg69N3Mi --var fates_pftname --silent  --val " broadleaf_colddecid_arctic_shrub" --nohist --overwrite --pft 11
output variable not interpretable as real. trying array
../tools/modify_fates_paramfile.py:84: DeprecationWarning: string or file could not be read to its end due to unmatched data; this will raise a ValueError in the future.
  outputval = np.fromstring(args.val, sep=',', dtype=np.float64)
Traceback (most recent call last):
  File "../tools/modify_fates_paramfile.py", line 355, in <module>
    main()
  File "../tools/modify_fates_paramfile.py", line 162, in main
    raise ValueError('variable dimension not handled in this script')
ValueError: variable dimension not handled in this script

@rgknox
Copy link
Contributor

rgknox commented Oct 3, 2024

@glemieux , if your stuck, can you print out the value of the string:

print("args.val: --{}--".format(args.val))
outputval = np.fromstring(args.val, sep=',', dtype=np.float64)

@glemieux
Copy link
Contributor Author

glemieux commented Oct 3, 2024

@glemieux , if your stuck, can you print out the value of the string:

print("args.val: --{}--".format(args.val))
outputval = np.fromstring(args.val, sep=',', dtype=np.float64)

Weirdly, the np.fromstring call on the new pft varname string doesn't result in a length of zero, which spot testing with a toy case suggests it should. It's actually returning a single element array with a value of -1, which is odd 🤔, and printing that deprecation warning. As such, it never triggers an exception to cascade down into the next try level here and set rename_pft to True:

outputval = np.fromstring(args.val, sep=',', dtype=np.float32)
if len(outputval) == 0:
raise RuntimeError('output variable needs to have size greater than zero')
except:
if args.varname != 'fates_pftname':
raise RuntimeError('output variable not interpretable as real or array')
else:
rename_pft = True

That said, I think I have a fix in which just involves checking to see if we're modifying the fates_pftname a bit early in that routine.

@glemieux
Copy link
Contributor Author

glemieux commented Oct 3, 2024

@rgknox looks like the fix I brought in via 204a061 this works. Would you take a look and give this PR an approval if it looks good to you?

@glemieux
Copy link
Contributor Author

glemieux commented Oct 3, 2024

Regression testing on derecho is complete. There are no unexpected failures. All testmods have DIFFs against the baseline (with the exception of the one ctsm-only testmod), which is expected given the extent of the parameter file changes and noted in #1136 (comment). Spot checking the satellite phenology SMS test shows results similar to those found in #1236 (comment).

Testing on izumi is still needed.

@glemieux
Copy link
Contributor Author

glemieux commented Oct 4, 2024

Testing on izumi is underway.

@glemieux
Copy link
Contributor Author

glemieux commented Oct 4, 2024

Izumi testing is complete. There are no unexpected failures.

Location: /scratch/cluster/glemieux/ctsm-tests/tests_1004-141329iz

@glemieux glemieux merged commit 825579d into NGEET:main Oct 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants