Skip to content

Incorporate changes from PR #1564 into dynurb_mksurf_part2 branch#1587

Closed
olyson wants to merge 1 commit into
ESCOMP:dynurb_mksurf_part2from
olyson:dynurb_mksurf_ctsm5.2.mksurfdata_I1564
Closed

Incorporate changes from PR #1564 into dynurb_mksurf_part2 branch#1587
olyson wants to merge 1 commit into
ESCOMP:dynurb_mksurf_part2from
olyson:dynurb_mksurf_ctsm5.2.mksurfdata_I1564

Conversation

@olyson

@olyson olyson commented Dec 30, 2021

Copy link
Copy Markdown
Contributor

Description of changes

Incorporate changes from PR #1564 (Change renormalization of landunit areas in mksurfdata_map to be consistent with raw datasets) into dynurb_mksurf_part2 branch.

Specific notes

Contributors other than yourself, if any: @billsacks

CTSM Issues Fixed (include github issue #):
Completely resolves #1555 (note that part of #1555 was split out into its own issue: #1556 )

Are answers expected to change (and if so in what way)? Yes, surface datasets will change.

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any:
See description of testing in PR #1564

@olyson olyson added bug something is working incorrectly PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete labels Dec 30, 2021
@olyson olyson requested a review from billsacks December 30, 2021 18:16
@olyson olyson changed the title Incorporate changes from PR #1564 Incorporate changes from PR #1564 into dynurb_mksurf_part2 branch Dec 30, 2021
@billsacks billsacks added PR status: later and removed PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete labels Jan 5, 2022

@billsacks billsacks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally got around to looking over this new version of #1564 . Thank you very much @olyson for doing this. It looks great to me. The only request for a change that I have is to edit the initial PR comment to clarify that this completely resolves #1555 other than the part that was split out into its own issue (#1556). You sort of say that, but let's state it more directly (if you agree that that's true), so that we can be sure to close #1555 when this is merged.

The original plan was to merge #1586 into the ctsm5.2.mksurfdata branch, then change the base of this PR to that branch, then do testing and merge this (see #1586 (comment) for details). But that plan has changed with the work @mvertens and @slevisconsulting have been doing to completely rewrite mksurfdata_map.

So, @mvertens and @slevisconsulting, I think the new plan is that you will make sure that all of the changes in #1586 and this PR have been incorporated into the new version of mksurfdata_map, at which point we can close this PR. Do you agree?

@olyson

olyson commented Mar 14, 2022

Copy link
Copy Markdown
Contributor Author

I've updated the initial PR comment.

@slevis-lmwg

Copy link
Copy Markdown
Contributor

So, @mvertens and @slevisconsulting, I think the new plan is that you will make sure that all of the changes in #1586 and this PR have been incorporated into the new version of mksurfdata_map, at which point we can close this PR. Do you agree?

@mvertens alerted me to this when I met with her today, and I'm fine with incorporating #1586 and #1587 in #1663. I will get to that in the next few days.

slevis-lmwg added a commit that referenced this pull request Mar 17, 2022
…_I1564' into feature/new_mksurfdata

This merge was discussed here:
#1587 (comment)
slevis-lmwg added a commit that referenced this pull request Mar 21, 2022
Repeated same tests as for the previous commit with same outcomes:
- non-transient PASS
- transient FAIL with same error as last time
@slevis-lmwg

Copy link
Copy Markdown
Contributor

@ekluzek
I saw that you merged #1586 to the first ctsm5.2.mksurfdata tag, and I wonder what should happen to this PR here.

@slevis-lmwg

Copy link
Copy Markdown
Contributor

...to explain why I asked the above question:
As with #1586 , I manually incorporated the code changes from #1587 in mksurfdata_esmf.

@ekluzek

ekluzek commented Apr 25, 2022

Copy link
Copy Markdown
Contributor

@slevisconsulting all of the work with mksurfdata should be rebased into the ctsm5.2.mksurfdata branch. That's where they'll actually get merged into and turned into alpha-ctsm5.2 tags. Since, mksurfdata_esmf is new code a lot of that work will need to be done manually. This one could still merge into dynurb_mksrf_part2, but dynurb_mksrf_part2 itself should be rebased to merge into ctsm5.2.mksurfdata.

@billsacks

Copy link
Copy Markdown
Member

@slevisconsulting says this has been manually incorporated into the 5.2 mksurfdata branch. @olyson will confirm. Assuming these changes are all on the 5.2 mksurfdata branch, we can close #1555.

@olyson

olyson commented Oct 13, 2022

Copy link
Copy Markdown
Contributor Author

I've reviewed this. The only difference I see is that I believe this line in the ctsm5.2 mksurfdata branch:

           write (6,*) subname, 'Error: sum of special land units nearly 100% but none is >= 1% at ', &

should be:

           write (6,*) subname, 'Error: sum of special plus crop land units nearly 100% but none is >= 1.0% at ', &

given that suma is the sum of special plus crop.
@slevisconsulting do you agree?

@slevis-lmwg

Copy link
Copy Markdown
Contributor

I've reviewed this. The only difference I see is that I believe this line in the ctsm5.2 mksurfdata branch:

           write (6,*) subname, 'Error: sum of special land units nearly 100% but none is >= 1% at ', &

should be:

           write (6,*) subname, 'Error: sum of special plus crop land units nearly 100% but none is >= 1.0% at ', &

given that suma is the sum of special plus crop. @slevisconsulting do you agree?

@olyson thank you for checking and good catch! I will add this to a list of small corrections needed in the ctsm5.2 branch. At this morning's meeting I agreed to open a ctsm5.2 PR. I will address items on that list there.

And otherwise, feel free to close this PR (#1587).

@olyson

olyson commented Oct 13, 2022

Copy link
Copy Markdown
Contributor Author

Sam is fixing the item I identified on the branch. So I am closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something is working incorrectly

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

4 participants