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

ungridded dimension for pstokes #1591

Merged

Conversation

jiandewang
Copy link
Collaborator

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.

Copy link
Collaborator

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

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

@jiandewang
Copy link
Collaborator Author

jiandewang commented Feb 1, 2023

@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
Copy link
Collaborator

@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
Copy link
Collaborator

marshallward commented Feb 1, 2023

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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 NOAA-GFDL#270, right ?

@marshallward
Copy link
Collaborator

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.

Testing to see if GH actions is failing due to MPI installation
@codecov
Copy link

codecov bot commented Feb 1, 2023

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%) ⬆️
...parameterizations/vertical/MOM_diabatic_driver.F90 46.37% <0.00%> (+0.07%) ⬆️
src/framework/MOM_restart.F90 32.76% <0.00%> (+0.12%) ⬆️
src/core/MOM_dynamics_split_RK2.F90 63.71% <0.00%> (+0.15%) ⬆️
src/framework/MOM_diag_mediator.F90 58.83% <0.00%> (+0.18%) ⬆️
src/framework/MOM_checksums.F90 58.04% <0.00%> (+0.47%) ⬆️
src/framework/MOM_io.F90 33.26% <0.00%> (+0.49%) ⬆️
... and 3 more

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

Copy link
Collaborator

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

These changes fall outside the scope of what we use. Therefore these changes do not impact our answers, though they look okay and I am happy to see that if (cesm_coupled) has been removed.

These sort of blocks (or equivalent ifdef directives always come in the way of making our couplers more uniform and seamless!

@jiandewang
Copy link
Collaborator Author

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

Copy link
Collaborator

@kshedstrom kshedstrom left a comment

Choose a reason for hiding this comment

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

I can confirm that this does not break my code since I'm not linking in the changed files.

Copy link
Collaborator

@abozec abozec left a comment

Choose a reason for hiding this comment

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

COAPS does not use the nuopc_cap either so it won't have any effect on our configurations. I approve.

@Hallberg-NOAA
Copy link
Collaborator

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
Copy link
Collaborator

breichl commented Feb 3, 2023

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
Copy link
Collaborator

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

@jiandewang
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants