mom-ocean / MOM6

Modular Ocean Model
Other
185 stars 232 forks source link

ungridded dimension for pstokes #1591

Closed jiandewang closed 1 year ago

jiandewang commented 1 year ago

this is a minor PR to implements use of ungridded dimensions for the partitioned stokes drift by removing the exchange of 6 individual fields in favor of 2 fields containing 3 ungridded dimensions each.

The WW3 mesh cap has the capability to use ungridded dimensions to transfer fields. This allows the 6 fields (3 each for x,y) used to exchange the partitioned stokes drift between WW3 and MOM6 to be simplified to only 2 fields. The MOM6 and WW3 caps used in UFS can be simplified as a result.

jiandewang commented 1 year ago

@jiandewang These files are outside the scope of what we use (since they pertain to nuopc). For that reason, I'll decline reviewing this PR.

@marshallward would you please "adjust" that from 5 reviews policy if not enough folks review it. Thank you!

@sanAkel fully understand, I think @kshedstrom will be on the same boat as you. Let me add @gustavo-marques How do I remove Kate and @sanAkel ?

marshallward commented 1 year ago

@sanAkel your review is primarily an acknowledgement that the change will not impact your runs. Critical feedback is greatly appreciated, of course, but it's not necessary to understand everything coming in.

Which is to say, just click "approved" if it doesn't affect you :).

marshallward commented 1 year ago

The CI failures appear to be from mom-ocean/main, not EMC. Most likely due to software stack changes on GitHub's end.

Merging dev/gfdl into this PR does appear to work, so it's likely that the problem will go away after merging dev/gfdl into main. I would feel more comfortable fixing this before accepting this PR though, so we may want to cherry-pick a fix into either main dev/emc before accepting.

I can't see what the actual problem could be, however, so I'm not yet sure how to proceed.

Currently, the problem is a FMS library link failure. The FMS library can be built, and module include test passes (fms_mod.mod can be found and read by the compiler), but the link test fails because it cannot find any of the symbols.

jiandewang commented 1 year ago

The CI failures appear to be from mom-ocean/main, not EMC. Most likely due to software stack changes on GitHub's end.

Merging dev/gfdl into this PR does appear to work, so it's likely that the problem will go away after merging dev/gfdl into main. I would feel more comfortable fixing this before accepting this PR though, so we may want to cherry-pick a fix into either main dev/emc before accepting.

I can't see what the actual problem could be, however, so I'm not yet sure how to proceed.

Currently, the problem is a FMS library link failure. The FMS library can be built, and module include test passes (fms_mod.mod can be found and read by the compiler), but the link test fails because it cannot find any of the symbols.

@marshallward it might be simple to do cherry-pick into this PR branch but I need your instruction on how to do the cherry-pick

marshallward commented 1 year ago

The fix appears to be the change from MPICH to OpenMPI. I don't recall why this was needed, but I didn't think it was related to FMS linking.

Anyway, I agree with your suggestion to cherry-pick into the EMC pull request, I'll send you some instructions if needed.

jiandewang commented 1 year ago

The fix appears to be the change from MPICH to OpenMPI. I don't recall why this was needed, but I didn't think it was related to FMS linking.

Anyway, I agree with your suggestion to cherry-pick into the EMC pull request, I'll send you some instructions if needed.

@marshallward you mean https://github.com/NOAA-GFDL/MOM6/pull/270, right ?

marshallward commented 1 year ago

Yes, thats the one. (Looks like Alistair did not know why either.)

My guess: The MPI calls in FMS have some type mismatches, which is why we need the -fallow-argument-mismatch flag. Perhaps these still work in OpenMPI but not in MPICH.

I've sent some basic instructions, but get in touch if it doesn't work.

codecov[bot] commented 1 year ago

Codecov Report

Merging #1591 (8993fc0) into main (7467a63) will increase coverage by 0.03%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1591      +/-   ##
==========================================
+ Coverage   37.18%   37.22%   +0.03%     
==========================================
  Files         263      263              
  Lines       73035    73074      +39     
  Branches    13609    13608       -1     
==========================================
+ Hits        27161    27204      +43     
- Misses      40859    40864       +5     
+ Partials     5015     5006       -9     
Impacted Files Coverage Δ
src/tracer/MOM_generic_tracer.F90 0.00% <0.00%> (ø)
src/parameterizations/CVmix/cvmix_kpp.F90 0.00% <0.00%> (ø)
src/ice_shelf/MOM_ice_shelf_diag_mediator.F90 0.00% <0.00%> (ø)
src/core/MOM_barotropic.F90 58.98% <0.00%> (+0.01%) :arrow_up:
...parameterizations/vertical/MOM_diabatic_driver.F90 46.37% <0.00%> (+0.07%) :arrow_up:
src/framework/MOM_restart.F90 32.76% <0.00%> (+0.12%) :arrow_up:
src/core/MOM_dynamics_split_RK2.F90 63.71% <0.00%> (+0.15%) :arrow_up:
src/framework/MOM_diag_mediator.F90 58.83% <0.00%> (+0.18%) :arrow_up:
src/framework/MOM_checksums.F90 58.04% <0.00%> (+0.47%) :arrow_up:
src/framework/MOM_io.F90 33.26% <0.00%> (+0.49%) :arrow_up:
... and 3 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

jiandewang commented 1 year ago

Yes, thats the one. (Looks like Alistair did not know why either.)

My guess: The MPI calls in FMS have some type mismatches, which is why we need the -fallow-argument-mismatch flag. Perhaps these still work in OpenMPI but not in MPICH.

I've sent some basic instructions, but get in touch if it doesn't work.

thanks for Marshall's help, this PR branch has cherry-picked Alistair's fixing on CI issue

Hallberg-NOAA commented 1 year ago

I am not seeing anything here that I expect would present a problem from the GFDL side, especially as all these changes are confined to the NUOPC coupler, but given the potentially strong interactions with the wave interfaces, I would like to defer formal approval from GFDL until we hear what @breichl thinks about this PR.

breichl commented 1 year ago

I am not seeing anything here that I expect would present a problem from the GFDL side, especially as all these changes are confined to the NUOPC coupler, but given the potentially strong interactions with the wave interfaces, I would like to defer formal approval from GFDL until we hear what @breichl thinks about this PR.

I don't foresee any conflicts with our use of the wave interfaces from the proposed code updates. I recommend approving from GFDL.

marshallward commented 1 year ago

This has passed our CI and review; GFDL approves this PR.

jiandewang commented 1 year ago

since we got green lights from all reviewers, I am going to merge it soon.