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

Cleanup of CatchCN restart routines #629

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

gmao-jkolassa
Copy link
Contributor

@gmao-jkolassa gmao-jkolassa commented Mar 3, 2023

This PR addresses GEOSgcm_GridComp issue #667 (https://github.com/GEOS-ESM/GEOSgcm_GridComp/issues/667) and is paired with GEOSgcm_GridComp PR #682 (https://github.com/GEOS-ESM/GEOSgcm_GridComp/pull/682).

It removes the duplication of Catchment-CN specific constants in CatchmentCNRst.F90 and instead imports them through use-statements. It also removes the duplication of orbital constants and replaces the use of hard-coded orbital subroutines with the relevant MAPL subroutines.

This PR should be zero-diff for Catchment and is zero-diff for Catchment-CN when used with RESTART: 1. It is not zero-diff for Catchment-CN used with RESTART: 2. When running Catchment-CN with RESTART: 2, there are differences in the day length variable (dayx) with maximum differences on the order of 10 seconds.

UPDATED 7 Mar 2023 by @gmao-rreichle:
PR is not 0-diff for diagnostic (HISTORY) output from all current "model" GEOSldas tests because of the update to MAPL v2.35.2.
MAPL v2.35 fixed a bug in the bit shaving tool, and the GEOSldas "model" tests happen to use bit shaving.
Restarts are 0-diff (except for simulations with CatchCN using RESTART: 2 as noted above.

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This PR is being prevented from merging because you have added one of our blocking labels: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. You'll need to remove it before this PR can be merged.

1 similar comment
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This PR is being prevented from merging because you have added one of our blocking labels: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. You'll need to remove it before this PR can be merged.

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This PR is being prevented from merging because you have not added one of our required labels: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Please add one so that the PR can be merged.

2 similar comments
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This PR is being prevented from merging because you have not added one of our required labels: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Please add one so that the PR can be merged.

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This PR is being prevented from merging because you have not added one of our required labels: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Please add one so that the PR can be merged.

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This PR is being prevented from merging because you have added one of our blocking labels: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. You'll need to remove it before this PR can be merged.

@github-actions
Copy link

github-actions bot commented Mar 6, 2023

This PR is being prevented from merging because you have added one of our blocking labels: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. You'll need to remove it before this PR can be merged.

@biljanaorescanin
Copy link
Contributor

Nightly tests summary @gmao-rreichle, @weiyuan-jiang :

Comparing LDAS output for nc4 files failed, almost all vars in "lnd" and "lfs" collections differ (for global run 48/56 differ):

Screen Shot 2023-03-04 at 10 09 47 PM

For example SFMC maximum with branch went up for 0.00008. All looks in realm of roundoff.

Comparing for restarts is zero diff.

GEOSgcm develop vs branch (with new MAPL) is zero diff.

@gmao-jkolassa
Copy link
Contributor Author

Just adding for extra information that the paired GEOSgcm_GridComp branch lacked an update to the most recent 'develop' branch reflecting changes that Anton made. This is likely the cause of the differences. I just merged develop into my branch again and we are currently re-running the tests.

@gmao-rreichle
Copy link
Contributor

Not sure why Anton's changes (new IGNI GridComp) should give us non-0-diff with GEOSldas runs. It was successfully tested 0-diff for the GCM (I assume) and should have no effect unless DO_FIRE_DANGER is set to .true.
I wonder if the newer MAPL version might be the cause of the diff.

@biljanaorescanin
Copy link
Contributor

I've run clean set of tests now, after Jana's update to sync with develop, and with MAPL v2.35.1.
One test is done, "CONUS" and it failed, same way for "lnd" and "lfs" comparison.
So these fails are what we have with changes we did.

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin, please run a GEOSldas test using GEOSldas "develop" with just one change -- to MAPL 2.35. That'll tell is if it's Jana's change or MAPL. I fear it's the latter.

@biljanaorescanin
Copy link
Contributor

Yeah, Jana suggested same thing I am setting that now. Stay tuned.

@biljanaorescanin
Copy link
Contributor

biljanaorescanin commented Mar 6, 2023

CONUS just failed for only MAPL v2.35.1 and develop, it ailed same way as on the branch for "lnd" and "lfs" comparisons. I will leave all tests to finish but I expect all fails we have seen here are due to MAPL.

Not sure but few things here could be candidates for changes we see @weiyuan-jiang:
https://github.com/GEOS-ESM/MAPL/releases/tag/v2.35.0

@gmao-rreichle
Copy link
Contributor

@mathomp4, @weiyuan-jiang:

CONUS just failed for only MAPL v2.35.1 and [GEOSldas] develop [..] for "lnd" and "lfs" comparisons. [..] Not sure but few things here could be candidates for changes we see @weiyuan-jiang: https://github.com/GEOS-ESM/MAPL/releases/tag/v2.35.0

MAPL v2.35.1 (and presumably also v2.35.0) is apparently not zero-diff for GEOSldas, contradicting the MAPL release notes. I suspect we skipped testing MAPL v2.35 with GEOSldas. Or didn't we?

The challenge now is to understand which of the MAPL changes caused the diffs, to determine if the revisions in MAPL can/should be zero-diff by patching MAPL v2.35, and -- if non-0-diff is the only option -- to verify that the diffs are acceptable ("roundoff").

@biljanaorescanin
Copy link
Contributor

I didn't test MAPL v2.35.0.

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

This PR is being prevented from merging because you have added one of our blocking labels: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. You'll need to remove it before this PR can be merged.

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

This PR is being prevented from merging because you have added one of our blocking labels: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. You'll need to remove it before this PR can be merged.

1 similar comment
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

This PR is being prevented from merging because you have added one of our blocking labels: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. You'll need to remove it before this PR can be merged.

@gmao-rreichle gmao-rreichle marked this pull request as ready for review March 7, 2023 20:20
@gmao-rreichle gmao-rreichle requested a review from a team as a code owner March 7, 2023 20:20
@gmao-rreichle gmao-rreichle merged commit b9f08c4 into develop Mar 7, 2023
@gmao-rreichle gmao-rreichle deleted the feature/jkolassa_catchCNrst_cleanup branch March 7, 2023 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants