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

change needed for the addition of a dglc component in cdeps #2449

Merged
merged 7 commits into from
Apr 4, 2024

Conversation

mvertens
Copy link

@mvertens mvertens commented Mar 30, 2024

Description of changes

change needed for the addition of a dglc component in cdeps

Specific notes

This minor change to buildnml is needed in order to support the addition of dglc which will replace CISM running in noevolve mode.
This will enable having many of the current CTSM tests that use SGLC move to using DGLC%NOEVOLVE.

Contributors other than yourself, if any: None

CTSM Issues Fixed: None

Are answers expected to change (and if so in what way)? No

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

Testing performed, if any:
Ran a test case with
compset 1850_DATM%GSWP3v1_CLM50%SP_SICE_SOCN_SROF_DGLC%NOEVOLVE_SWAV and resolution f10_f10_ais8gris4_mg37
and compared to
compset 1850_DATM%GSWP3v1_CLM50%SP_SICE_SOCN_SROF_CISM%NOEVOLVE_SWAV and resolution f10_f10_ais8gris4_mg37

@mvertens mvertens requested a review from billsacks March 30, 2024 09:20
@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability tag: simple bfb labels Mar 31, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Apr 1, 2024

@mvertens thanks so much for working on this and bringing this in. I really like the idea of having a data GLC model to take the place of CISM NO-EVOLVE mode. This also resolves #2222 for us which is really important.

But, I have some questions for you on this:

  • It looks like this is a work in progress correct? How close is this to being considered done by you?
  • Will it require a CDEPS update to bring in?
  • Right now this won't change answers for anything existing correct?

Since this is adding new capability and is fairly straight forward (and provided it doesn't change answers otherwise)-- I think I should rebase this to the b4b-dev branch. So I'll do that once you verify Doing that means it's easier to get in sooner. I'll work with you to do that once I hear more about this from the questions above.

I'm also going to make an issue about adding DGLC to CTSM which will allow us to close #2222 and track everything that will need to be done for this change. Again I'll wait until hearing and then I'll have you add anything there that I'm missing after the issue is started...

@billsacks
Copy link
Member

@ekluzek - Thanks a lot for looking at this and for your good questions. I have been talking with @mvertens about this, so I'll respond here, and I can be the point person on this. There is a separate (big) PR to CDEPS from @mvertens to add the data glc model (ESCOMP/CDEPS#268); this CTSM PR is just a small piece that's needed to coordinate with that. This CTSM PR will require a small CISM update (to use GLC_USE_ANTARCTICA instead of CISM_USE_ANTARCTICA); I'm going to submit a PR for that shortly, and will work with @Katetc to bring that in soon. Besides that, this CTSM PR is ready to come in.

This does not need a CDEPS update. Eventually (before too long, but not immediately) we will want to create I compsets that use dglc, but that can / should be a later step.

Right that this won't change answers, as long as it pulls in the small CISM update.

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 1, 2024

Thanks for that update @billsacks. So there is only one more thing needed here and that is an update for CISM (once that is done). Other things will be done in future PR's and we can document those in the issue you created #1136.

So things needed for this:

Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

This is really great glad to see this coming in.

The code changes right now are very simple, so not much to review. But, having both DGLC and CISM use the same XML variable for similar behavior is a nice improvement.

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 1, 2024

@billsacks can you comment on how much of our testing should change from using the stub-glacier model to DGLC? Are there reasons outside of fully coupled B1850 compsets that we should prefer running with DGLC over SGLC? Scientifically it's obviously an update to use DGLC rather than a stub, but in what ways will this matter for CTSM simulations?

@wwieder
Copy link
Contributor

wwieder commented Apr 1, 2024

Glad to see this can come in on b4b_dev. Is that we want to get in this week, or can it wait another 2 weeks? I guess the logistics are a question for @billsacks and @ekluzek . The urgency is more a question for @mvertens and @Katetc?

@billsacks
Copy link
Member

can you comment on how much of our testing should change from using the stub-glacier model to DGLC? Are there reasons outside of fully coupled B1850 compsets that we should prefer running with DGLC over SGLC? Scientifically it's obviously an update to use DGLC rather than a stub, but in what ways will this matter for CTSM simulations?

I'll comment on that over in #1136

Glad to see this can come in on b4b_dev. Is that we want to get in this week, or can it wait another 2 weeks? I guess the logistics are a question for @billsacks and @ekluzek . The urgency is more a question for @mvertens and @Katetc?

It would be great to get this in this week, since this is required for the CDEPS change that we want to merge.

@ekluzek ekluzek changed the base branch from master to b4b-dev April 1, 2024 21:06
@ekluzek
Copy link
Collaborator

ekluzek commented Apr 1, 2024

OK in that case I've rebased it to b4b-dev and as long as we get the CISM tag in by Wednesday morning we can bring it into b4b-dev, which will then go to master on Thursday.

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 4, 2024

Testing for aux_clm looks as expected. So merging.

@ekluzek ekluzek merged commit d79c236 into ESCOMP:b4b-dev Apr 4, 2024
2 checks passed
@ekluzek ekluzek deleted the feature/add_dglc branch April 4, 2024 04:55
@adrifoster adrifoster mentioned this pull request Apr 5, 2024
@ekluzek ekluzek self-assigned this Apr 5, 2024
@samsrabin samsrabin added simple bfb bit-for-bit science Enhancement to or bug impacting science labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit enhancement new capability or improved behavior of existing capability science Enhancement to or bug impacting science
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants