ESCOMP / MOSART

Model for Scale Adaptive River Transport, Mosart, part of the Community Earth System Model
http://www.cesm.ucar.edu/
Other
8 stars 27 forks source link

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

Closed mvertens closed 3 months ago

mvertens commented 4 months ago

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

Issues resolved:

TODOs:

mvertens commented 4 months ago

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

I have spoken with @jedwards4b - and created the issue https://github.com/ESCOMP/MOSART/issues/100. I will not be implementing these changes in this PR.

mvertens commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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

mvertens commented 3 months ago

@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 commented 3 months ago

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

billsacks commented 3 months ago

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

slevis-lmwg commented 3 months ago

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 commented 3 months ago

Oh, right, and i have confirmed that we need to complete https://github.com/ESCOMP/CTSM/issues/2590 for the tests to pass.

jedwards4b commented 3 months ago

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.

mvertens commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

slevis-lmwg commented 3 months ago

Made a new tag:

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