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

cism runoff will be now routed to ocn via mosart #94

Merged
merged 20 commits into from
Jun 21, 2024

Conversation

mvertens
Copy link
Contributor

@mvertens mvertens commented Jun 6, 2024

This PR enables CISM runoff to be routed to the ocean via mosart.

  • All runoff from CISM will be routed directly to the outlet points
  • New fields will be advertised in the mosart cap
    call fldlist_add(fldsFrRof_num, fldsFrRof, 'Forr_rofl_glc')
    call fldlist_add(fldsFrRof_num, fldsFrRof, 'Forr_rofi_glc')

Issues resolved:

TODOs:

@mvertens mvertens added this to the CESM3 Capability Freeze milestone Jun 6, 2024
@mvertens mvertens linked an issue Jun 6, 2024 that may be closed by this pull request
Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

I recommend removing all direct references to mpi and using ESMF_VM calls instead - this future proofs the code a little in case the mpi library is replaced with some other comm library on some future hardware - if all our calls are through the esmf layer then only esmf needs to be updated.

src/cpl/nuopc/rof_comp_nuopc.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_budget_type.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_budget_type.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_control_type.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_control_type.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_control_type.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_driver.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_driver.F90 Outdated Show resolved Hide resolved
@mvertens
Copy link
Contributor Author

mvertens commented Jun 7, 2024

I recommend removing all direct references to mpi and using ESMF_VM calls instead - this future proofs the code a little in case the mpi library is replaced with some other comm library on some future hardware - if all our calls are through the esmf layer then only esmf needs to be updated.

I have concerns about this (also listed below).

  • ESMF_VMAllReduce only accepts one-dimensional arrays. tmp_in and tmp_glob are 2d-arrays. So copies will need to be made and 6 calls will need to be done and then copies need to be made. Frankly, I am worried that moving to only ESMF MPI communication will make the code harder to understand since it really supports only a very small subset of MPI communications. And I think it is not a good idea to mix ESMF calls in the code with MPI calls. Happy to talk about this some more.
  • Using ESMF_VMBroadcast - originally I thought this was a good idea. Looking at this some more I found that ESMF_VMBroadcast only accepts arrays. That means that an array of length 1 needs to be created and then a copy of finidat needs to be made to that and then broadcast and then copied back. I think this will make the code more complicated and harder to read.

I have spoken with @jedwards4b - and created the issue #100.
I will not be implementing these changes in this PR.

Copy link
Contributor

@slevis-lmwg slevis-lmwg left a comment

Choose a reason for hiding this comment

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

@mvertens I saw your post, so I will go ahead and approve. I will coordinate the merge with @ekluzek to make sure that I handle this comment correctly:
"This PR must also be accompanied by CISM, CMEPS, ccs_config PRs (to be listed here as they are generated)."

@mvertens
Copy link
Contributor Author

mvertens commented Jun 7, 2024

@slevis-lmwg - I still need to run the mosart test suite and make sure that everything is bfb. So no hurry to approve just yet.

@slevis-lmwg
Copy link
Contributor

@slevis-lmwg - I still need to run the mosart test suite and make sure that everything is bfb. So no hurry to approve just yet.

@mvertens will you merge mosart1.1.01 to this PR or would you like me to do it and, if the latter, then before or after you run the test suite?

@mvertens
Copy link
Contributor Author

mvertens commented Jun 7, 2024

@slevis-lmwg - I'll merge mosart1.1.0.1 to this PR before I run the test suite.

@mvertens mvertens requested a review from jedwards4b June 15, 2024 10:39
@mvertens
Copy link
Contributor Author

@slevis-lmwg - I think this is ready for merging based on all the tests I've done. @jedwards4b - do you want to review again?

@mvertens
Copy link
Contributor Author

@billsacks @Katetc - are you both okay with the current level of validation?

Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

lgtm

@billsacks
Copy link
Member

Yes, I'm satisfied with the current level of validation. Thank you very much for your work on this @mvertens !!!

@slevis-lmwg
Copy link
Contributor

I have checked out this branch and submitted the test-suites on izumi and derecho:
./run_sys_tests -s mosart -c mosart1.1.01_ctsm5.2.007 -g mosart1.1.02_ctsm5.2.007

@slevis-lmwg
Copy link
Contributor

Oh, right, and i have confirmed that we need to complete ESCOMP/CTSM#2590 for the tests to pass.

@jedwards4b
Copy link
Contributor

I think that there is going to be a bit of a chicken and egg problem here - you will need to use some PR branches to test some other PR's.

Copy link
Contributor

@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.

@mvertens thanks for all your work here. I have some updates that @slevis-lmwg can do, if you aren't able to do them. Or if you prefer to do them go ahead. Just let us know how to handle it.

One thing @slevis-lmwg realized when he started testing is that the separate_glc2ocn_fluxes namelist item assumes that ctl%rof_from_glc is true, and hence the code won't work with an older CISM version. In order to test this seperately it would be good to add some error checking that they are both true, and if not to abort. This also allows us to test with that flag to false and make sure it doesn't change answers, which is a good verification that the other refactoring is b4b.

So let us know how you want to handle the changes and we can continue forward.

cime_config/namelist_definition_mosart.xml Outdated Show resolved Hide resolved
src/cpl/nuopc/rof_import_export.F90 Show resolved Hide resolved
src/cpl/nuopc/rof_import_export.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_control_type.F90 Show resolved Hide resolved
src/cpl/nuopc/rof_import_export.F90 Show resolved Hide resolved
src/riverroute/mosart_control_type.F90 Show resolved Hide resolved
src/riverroute/mosart_driver.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_histfile.F90 Show resolved Hide resolved
src/riverroute/mosart_physics.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_budget_type.F90 Show resolved Hide resolved
@mvertens
Copy link
Contributor Author

@ekluzek @billsacks @slevis-lmwg - I have addressed the issues in this PR except for addressing refactoring the use of 'subname' and reintroducing empty routines whose presence were confusing.

@billsacks
Copy link
Member

It looks like @mvertens has addressed all of the critical points here; if so, it would be great if any non-critical pieces can be deferred to a follow-up PR. @ekluzek and @slevis-lmwg are there any other important needs that you see that need to be addressed before this can be brought in?

@ekluzek
Copy link
Contributor

ekluzek commented Jun 20, 2024

Thanks for working on this @mvertens and @billsacks -- yes this covers what we thought was important. This will also make @slevis-lmwg process easier to bring this part in.

@ekluzek
Copy link
Contributor

ekluzek commented Jun 20, 2024

One thing I will point out is that the changes as now implemented do require a CMEPS and CISM update. The logical ctl%rof_from_glc won't work correctly to use an older CISM version, because it's set too late. I'll make an issue regarding this and I'll assume that will be fixed in a later PR. It sounds like there is a plan for some follow on PR's as well.

@ekluzek
Copy link
Contributor

ekluzek commented Jun 20, 2024

OK I made an issue in #103 about the point I made just above.

Copy link
Contributor

@slevis-lmwg slevis-lmwg left a comment

Choose a reason for hiding this comment

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

With one izumi aux_clm test still in progress, I'm approving this PR.

The mosart test results appear in the umbrella PR here and can be summarized as follows:

  • derecho and izumi OK: NLCOMP and "field lists differ"
  • new baseline mosart1.1.02_ctsm5.2.007

For the aux_clm results, see the "umbrella" PR.

@slevis-lmwg slevis-lmwg merged commit e2ffe00 into ESCOMP:master Jun 21, 2024
@slevis-lmwg slevis-lmwg deleted the feature/cism2mosart branch June 21, 2024 23:38
@slevis-lmwg
Copy link
Contributor

Made a new tag:

git tag -a mosart1.1.02
git push escomp mosart1.1.02

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