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

EMC update 20210322 #1357

Merged
merged 42 commits into from
Mar 25, 2021
Merged

Conversation

marshallward
Copy link
Collaborator

@marshallward marshallward commented Mar 23, 2021

This PR is an update from EMC courtesy of @jiandewang.

We are merging this directly into dev/gfdl due to a mistake on our part where we prematurely merged dev/gfdl into main. These premature unreviewed commits ended up in dev/emc fork before we detected our mistake. Once approved for merge into dev/gfdl, we will then submit this to main for review ASAP.

Summary of changes (provided by @jiandewang):

  • fixed missing halo in two halo updates for taux and tauy in mom_surface_forcing_nuopc.F90 in nuopc_cap
  • allow MOM6 to use a mesh optionally in nuopc_cap
  • fixed sign error on fprec passed to ocean in mom_surface_forcing_nuopc.F90 in nuopc_cap (from NCAR)
  • allow ocean->land topo edits in MOM_shared_initialization.F90

DeniseWorthen and others added 30 commits November 26, 2019 09:08
- In A and B grid configuration halos were never updated after taux/tauy were populated.
- This propogated through to the ustar_gustless field, hence caused a restart issue when using ustar_gustless in parameterizations.
- This appears to correct the restart issue by updating the halos at the end of the A and B grid taux/tauy loops.
…s_in_nupoc

Add two halo updates for taux and tauy in mom_surface_forcing_nuopc
…20210120

This is corresponding GFDL 20210120 main branch commit (hash # fe5e605)
This patch fixes a sign bug, in both MCT and NUOPC, when
accounting for the latent heat from fprec and frunnoff.
Following MOM6's definition, both fprec
and frunoff are > 0 into the ocean. Therefore, the latent heat
associated with these terms should be negative.
Bugfix: sign error on fprec for nuopc and mct caps
…L-20210120

Feature/update to gfdl 20210120
* read config variable 'use_mommesh' in ufs; default is false
to have mom cap run on grid
@marshallward
Copy link
Collaborator Author

marshallward commented Mar 23, 2021

https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/12325 ✔️ 🟡

This has passed but will require a parameter file update (ALLOW_LANDMASK_CHANGES).

@marshallward
Copy link
Collaborator Author

@gustavo-marques Do you mind quickly reviewing this? I am going to send it to main as well but would be best to get your feedback now.

Copy link
Collaborator

@Hallberg-NOAA Hallberg-NOAA 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 answer-changing bug fix in the sign of frozen precipitation contributions to the latent heat fluxes, and I agree that this is a bug. We need to either get explicit buy-in from absolutely everyone that they want the code to irretrievably change answers using the mct and nuopc caps, without the ability to recreate the current answers, or we need to use a run-time flag to correct this bug.

If there any published papers that use the current (buggy) version, or even experimental runs that people find interesting, I strongly suspect that we will need a run-time bug-fix flag for these changes, and the default at first should be to keep the bug. We can move aggressively to change the default, and then more deliberately to obsolete and eliminate the bug-fix flag.

@jiandewang
Copy link
Collaborator

jiandewang commented Mar 25, 2021

@marshallward In your branch,

https://github.com/CVMix/CVMix-src/blob/9423197f894112edfcb1502245f7d7b873d551f9/src/shared/cvmix_tidal.F90#L807-L823

here rho is not defined as optional, but in

https://github.com/marshallward/MOM6/blob/emc_update_20210322/src/parameterizations/vertical/MOM_tidal_mixing.F90#L883-L887

you removed rho

@marshallward sorry the above might be a false alarm, I was not looking at the right subroutine in CVmixing

@marshallward
Copy link
Collaborator Author

@jiandewang dev/gfdl recently updated its CVMix source, so it would be good for you to check that there were no consequences. (I didn't handle this but it did not appear to be answer-changing).

@gustavo-marques
Copy link
Collaborator

This PR is changing answers for us but I do not believe the reason is the bug fixes in MCT and NUOPC. The answer-changing bug fixes are already implemented in dev/ncar (NCAR#174). We have notified, ~ 2 months ago, people that we know are affected by these changes. They were all okay with them.

We need to do a more thorough evaluation to find out what commit(s) is(are) changing answers. I will start by evaluating dev/gfdl. In terms of the changes being proposed here, I approve this PR.

@jiandewang
Copy link
Collaborator

@gustavo-marques let me also try dev/gfdl in UFS. (BTW, I will be out of town 27-31, so no need to be hurry)

@marshallward
Copy link
Collaborator Author

Thanks @gustavo-marques @jiandewang

@jiandewang : I'd like to merge this into dev/gfdl and hopefully send to main some time today.

I don't mind waiting if you prefer, but you will also get a chance to test this during the dev/gfdl -> main review period (where there is no hurry) so that would also work for us.

@jiandewang
Copy link
Collaborator

jiandewang commented Mar 25, 2021

@marshallward then do as you planned
@gustavo-marques I merged dev/gfdl to dev/emc and made a test run in UFS, first several jobs finished without answer change, other jobs are waiting in the queue, shall have black and white answer in 2 hours.
update: all UFS jobs finished without results change

@marshallward
Copy link
Collaborator Author

Great, thanks all. We will merge this to dev/gfdl ASAP and then will resolve any answer changes in the dev/gfdl -> main candidate.

@Hallberg-NOAA
Copy link
Collaborator

I am withdrawing my requested changes because everyone we are aware of who might be using the NUOPC or MCT couplers has indicated that they are on-board with these changes, some of which are already on the dev/ncar branch.

@Hallberg-NOAA Hallberg-NOAA dismissed their stale review March 25, 2021 17:51

We have had an indication that everyone who might be using this code is OK with this bug-fix, without introducing a run-time flag to recreate the bug.

@adcroft adcroft merged commit b92d763 into mom-ocean:dev/gfdl Mar 25, 2021
@marshallward marshallward deleted the emc_update_20210322 branch May 7, 2021 02:51
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.

7 participants