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

major mosart refactor including addition of new halo capability #76

Merged
merged 37 commits into from
Jun 6, 2024

Conversation

mvertens
Copy link
Contributor

@mvertens mvertens commented Dec 31, 2023

Removed all references to rtm

  • files have been renamed and namelists no longer contain rtm in the name

New modularity:

  • introduced new modules with new derived types and methods
    • mosart_control_type.F90
    • mosart_tctl_type.F90
    • mosart_tparameter_type.F90
    • mosart_tspatialunit_type.F90
    • mosart_tstatusflux_type.F90
  • the new modules modularize a lot of the complexity and variables that were previously found in RunOffMod.F90 and permit decomposition initialization to be more flexible and transparent.

New halo capability

  • Ability to have halo regions and communication using ESMF. This is needed for computing derivatives in upcoming new additions to MOSART.
  • New halo namelist - use_halo_option. When this is set to true halos can be activated. See the test_halo subroutine in mosart_control_type.F90 module.
  • Verified that the results for the halos are bfb identical regardless of the number of processors that are used.
    -To set the values for the exclusive region that will be used in halo operations - you need to access the pointer as is done in the test_halo routine in mosart_control_type.F90:
     n = 0
      do nr = this%begr,this%endr
         n = n + 1
         this%halo_arrayptr(n) = this%latc(nr)*10. + this%lonc(nr)/100.
      end do

      call ESMF_ArrayHalo(this%haloArray, routehandle=this%haloHandle, rc=rc)
      if (chkerr(rc,__LINE__,u_FILE_u)) return

Issues addressed:
Fixes #93
Fixes #98
Fixes #97
Fixes #99

Testing on derecho:

  • Verified that the following tests passed and were bfb with master
    • ERP_Ld3.f19_f19_mtn14.2000_DATM%GSWP3v1_CLM50%SP_SICE_SOCN_MOSART_SGLC_SWAV
    • SMS_D_Ln5.f19_f19_mtn14.2000_DATM%GSWP3v1_CLM50%SP_SICE_SOCN_MOSART_SGLC_SWAV
    • In both of the above use_halo_option was set to .true. in user_nl_mosart.
  • Verified that answers did not change with respect to mosart1_0_49
    • generated baselines xml-category mosart using cesm2_3_beta17 (baseline directory is called mosart1_0_49)
    • modified cesm2_3_beta17 to use mosart branch feature/refactor and reran the mosart tests - all results were bfb

mvertens and others added 3 commits December 24, 2023 16:18
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.

Throughout the code: hardcoded string lengths should be replaced with shr_kind parameters and use statements should have only clauses. I realize that much of this code is historic, but because the filenames were changed and the history was broken future developers may not realize that.

cime_config/namelist_definition_mosart.xml Outdated Show resolved Hide resolved
src/riverroute/mosart_fileutils.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_histfile.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_histfile.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_io.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_restfile.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_timemanager.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_timemanager.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_tspatialunit_type.F90 Outdated Show resolved Hide resolved
src/riverroute/mosart_vars.F90 Outdated Show resolved Hide resolved
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.

This is an excellent refactor. Thank you so much.

@ekluzek
Copy link
Contributor

ekluzek commented Jan 3, 2024

@mvertens can you tell us more about the new capability that is planned to MOSART after this refactor. For CESM4 we are hoping that we can deprecate and eliminate the use of MOSART in CESM and ONLY use mizuRoute.

A big limitation of MOSART is that we don't have tools to make new input datasets nor people we can rely on to make the datasets. This means MOSART can NOT be used for Paleo work for example, which kills it's usage for CESM4. At NCAR we don't have local experts for MOSART only for mizuRoute. Removing RTM and MOSART for CESM4 would allow us to only support one river model for CESM4 which would lower the projects maintenance costs.

Have you looked into adding these same changes to mizuRoute instead of (or in addition to) MOSART? We should at least look into that.

@mvertens
Copy link
Contributor Author

mvertens commented Jan 4, 2024

@ekluzek - the key capability that was needed is halo updates. This is currently needed by @swensosc for his work and he was targeting MOSART. I think doing this in MOSART was very easy and MOSART is the code base that will be supported in both NorESM3 and CESM3. So having a clean refactored code will be helpful to both efforts in my opinion. As for MizuRoute - I'm happy to talk about that in the future.

@mvertens mvertens mentioned this pull request Jan 6, 2024
1 task
@ekluzek
Copy link
Contributor

ekluzek commented Jan 8, 2024

I think this is a big enough of a change that we should bump the middle number in the version sequence.
And we might as well make the name more similar to CTSM tags.

So this becomes

mosart1.1.dev01

@jedwards4b
Copy link
Contributor

I have to say - I don't like the 'dev' part. Just make it mosart1.1.01

@ekluzek
Copy link
Contributor

ekluzek commented Jan 10, 2024

In talking about this today, there's one bit of additional functionality that will be added (having to do with gradients). This tag will be the basis for the GW branch, as well as the DOM work that is being done at NIO.

We also decided that this would be done after the list of simple PR's that will be done first.

@mvertens
Copy link
Contributor Author

@slevis-lmwg @ekluzek @jedwards4b - I have verified that this branch does not change answers with respect to mosart1_0_49. @mvdebolskiy also participated in updating this refactor branch to have updated internal diagnostics. This branch will be the starting point of the new glc->rof coupling that will replace glc->ocn.

@ekluzek
Copy link
Contributor

ekluzek commented Jun 3, 2024

The plan right now is for @slevis-lmwg to bring this in after he has some time clear with bringing CN Matrix to CTSM.

@slevis-lmwg
Copy link
Contributor

OK derecho test-suite
FAIL izumi test-suite during build; I have restarted the build in case it was a glitch; see here in case anyone wishes to take a look before Erik and I get to it:
/fs/cgd/data0/slevis/git_externals/major_refactor/tests_0605-174443iz

@mvertens
Copy link
Contributor Author

mvertens commented Jun 6, 2024

@slevis-lmwg - there were indeed compilation problem with nag for this PR. I have fixed the problems and confirmed that mosart can now build with nag on izumi. I have pushed the changes back.

@mvertens
Copy link
Contributor Author

mvertens commented Jun 6, 2024

@slevis-lmwg - I have addressed #93 and pushed the changes back.
It ends up I had to revert these changes since adding the PIO_BCAST_ERROR call caused the model to crash with a segmentation fault. @jedwards4b - can you please help with this.
@slevis-lmwg @jedwards4b - I think I know the problem. You don't get an file descriptor until you actually create or open the file. So you cannot call PIO_BCAST_ERROR before those calls - since that doesn't make sense.
@slevis-lmwg - I have now fixed these issues based on input from @jedwards4b .

@ekluzek
Copy link
Contributor

ekluzek commented Jun 6, 2024

Awesome, thanks for working on that @mvertens and being able to get it done quickly. We opened the issue in case this was a longer term thing that didn't need to be handled now. We figured it wasn't required for the Science capability freeze timeline, so a longer term issue also made sense. But, even better that you've already resolved it.

@slevis-lmwg
Copy link
Contributor

slevis-lmwg commented Jun 6, 2024

I reran the mosart test-suite:
OK on izumi, only NLCOMP diffs
OK on derecho, only NLCOMP and an EXPECTED FAILURE

UPDATE:
I reran the izumi test-suite for sanity, because I pushed one more commit after the last testing.

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 your work on all of this. These are great improvements. Nice, to see that all of this is bit-for-bit. I didn't go over this as extensively as @jedwards4b and we are grateful for his careful feedback here.

I looked at some big picture things that were happening here, and made issues to document what was done. In the future if you can make issues on these type of refactor type things you are doing that's helpful for us to discuss them as a group before they are finished and the code and testing completed. It also gives us pre-warning of work that is happening. It's unlikely to collide with something we are doing, but really helpful for discussion and planning.

Thanks for the work here and in the next PR

@slevis-lmwg slevis-lmwg merged commit 5acf6b2 into ESCOMP:master Jun 6, 2024
@slevis-lmwg slevis-lmwg deleted the feature/major_refactor branch June 6, 2024 21:45
@jedwards4b
Copy link
Contributor

@slevis-lmwg This repository does not have automated tag creation - will you make a new tag?

@slevis-lmwg
Copy link
Contributor

I type git tag from my latest master (local), and I get mosart1.1.01
which is the annotated that I created and pushed to escomp.

On github I see the same tag here:
https://github.com/ESCOMP/MOSART/tags

Please let me know if I'm not answering your question.

@jedwards4b
Copy link
Contributor

I think that you must have made the tag shortly after I posted that message - thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
7 participants