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

GFDL generic_tracers with MOM6 NUOPC cap #90

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

dougiesquire
Copy link
Collaborator

@dougiesquire dougiesquire commented Jan 15, 2024

This PR is a WIP to enable GFDL generic_tracers to be used with ACCESS-OM3.

The important changes in this PR are in the MOM6 directory. Currently, edits to CMEPS and CDEPS are also included to add oxygen to the jra55do datm datamode, but these were mostly done to familiarize myself with the codebases and will likely be removed. UPDATE: I've now reverted my changes to CMEPS and CDEPS.

Details of the changes to MOM6 can be found in MOM6/generic_tracer_nuopc.md.

For a corresponding ACCESS-OM3 configuration, see here. A few changes to Payu are required to run - see here.

Progress towards #42

@dougiesquire dougiesquire marked this pull request as draft January 15, 2024 04:19
Copy link
Contributor

@micaeljtoliveira micaeljtoliveira left a comment

Choose a reason for hiding this comment

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

I've had a look at the whole set of changes. Overall all looks reasonable. In the process I noticed a few things and left some comments.

A more general question: are the changes to CDEPS and CMEPS generic enough that they could be accepted upstream?

.gitmodules Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
MOM6/patches/mom_ocean_model_nuopc.F90.patch Show resolved Hide resolved
MOM6/patches/mom_cap.F90.patch Outdated Show resolved Hide resolved
MOM6/extra_sources/mom_cap_gtracer_flux.F90 Outdated Show resolved Hide resolved
MOM6/extra_sources/mom_cap_gtracer_flux.F90 Show resolved Hide resolved
@dougiesquire
Copy link
Collaborator Author

Thanks for taking a look @micaeljtoliveira!

A more general question: are the changes to CDEPS and CMEPS generic enough that they could be accepted upstream?

They're generic, but I doubt they'd be wanted. But as I mentioned in the PR description, I will probably remove these changes anyway. All they do is add oxygen to the jra55do datm datamode, mostly as a learning exercise for myself and as a test of the coupling I added. I doubt we want to keep this - it's probably easier/cleaner to prescribe oxygen using the data override stuff I added, rather than including a new stream in our datm.

@dougiesquire
Copy link
Collaborator Author

@angus-g, you mentioned you might be able to take a look at this. If you have time, any feedback would be much appreciated!

@access-hive-bot
Copy link

This pull request has been mentioned on ACCESS Hive Community Forum. There might be relevant details there:

https://forum.access-hive.org.au/t/cosima-twg-meeting-minutes-2023/399/17

@access-hive-bot
Copy link

This pull request has been mentioned on ACCESS Hive Community Forum. There might be relevant details there:

https://forum.access-hive.org.au/t/cosima-twg-meeting-minutes-2024/1734/1

Copy link

@angus-g angus-g left a comment

Choose a reason for hiding this comment

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

All in all, this looks fairly good, in my opinion. Most of the heavy lifting seems to be done by existing code lifted from the FMS coupler, and just a bit of glue in the NUOPC layer to get it working.

My only real concern would be about keeping in sync with the FMS coupler for any bugfixes, if required? Having said that, this is probably still the best way of going about it, rather than pulling in the entire coupler as a dependency!

MOM6/patches/mom_cap.F90.patch Show resolved Hide resolved
Comment on lines +88 to +100
+ ! Set NUOPC attribute additional_restart_dir to RESTART/ if not defined
+ additional_restart_dir = "RESTART/"
+ call NUOPC_CompAttributeGet(gcomp, name="additional_restart_dir", value=cvalue, &
+ isPresent=isPresent, isSet=isSet, rc=rc)
+ if (ChkErr(rc,__LINE__,u_FILE_u)) return
+ if (isPresent .and. isSet) then
+ additional_restart_dir = slasher(cvalue)
+ else
+ call ESMF_LogWrite('MOM_cap:additional_restart_dir unset. Defaulting to '//trim(additional_restart_dir), &
+ ESMF_LOGMSG_INFO)
+ endif
+ call NUOPC_CompAttributeSet(gcomp, name="additional_restart_dir", value=additional_restart_dir, rc=rc)
+ if (chkerr(rc,__LINE__,u_FILE_u)) return
Copy link

Choose a reason for hiding this comment

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

The "additional" restart dir is used because you're using FMS to write the tracer restart, rather than NUOPC? Maybe it's not important, but I wonder if it's worth a more explicit name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah (although NUOPC only really specifies how the files are handled, MOM does the writing). The name is general because it's a general config parameter that in theory it could be used by any model component handled by NUOPC. I'll see what I can do to make this clearer. One option is just to hardcode the directory (it wouldn't be the first).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main problem is that NUOPC wants the restart input and output directories to be ./ for all components. The restart filenames include a timestep to prevent files from being overwritten and a pointer file keeps track of the latest restart. The name of the additional restart file needed for the generic tracer fluxes is set within each generic_tracer module (i.e. I don't want to change it) and does not include a timestamp, so writing to and reading from MOM's restart_output_dir and restart_input_dir (=./), respectively, causes problems.

MOM6/patches/mom_cap_methods.F90.patch Outdated Show resolved Hide resolved
@dougiesquire
Copy link
Collaborator Author

dougiesquire commented Jan 24, 2024

Thanks heaps for the review @angus-g. I share your concern about staying on top of changes to FMScoupler. But I'm not sure of a better approach. It makes me feel slightly better that the routines we're ripping don't seem to change very often: they haven't changed at all since the files were renamed 3 years ago (atmos_ocean_fluxes_calc.F90, atmos_ocean_dep_fluxes_calc.F90)

dougiesquire and others added 20 commits March 4, 2024 09:13
failing with "The domain associated with the file: does not have an io_domain."
The NUOPC cap appears to have been written from the MCT cap, which does not include changes needed to successfully register restarts for FMS coupler type. See NCAR/MOM6@73304eb
move new routines into new mom_cap_gtracer_flux.F90

now only mom_cap_gtracer_flux.F90 adds new calls directly into FMS

now only adding _USE_GENERIC_TRACER ifdefs to mom_cap.F90
…from FMScoupler

commit: eeadda8dc74f605e9cb3b03be8a43ab2ce698496
operate on 2D inputs, rather than 1D

add calculation for air_sea_deposition from atmos_ocean_dep_fluxes_calc.F90

multiply fluxes by ice_fraction input, rather than masking based on seawater input

use MOM over FMS modules where easy to do so

make tsurf input optional, as it is only used by a few implementations

use ind_runoff rather than ind_deposition in runoff flux calculation

rename gas_fields_ice to gas_fields_ocn

add param array into Ice_ocean_boundary%fluxes after spawning
pass optional atm_fields to mom_import as 2D BC type

do atm_fields override and flux calcs from mom_cap
spawn 2D coupler_type for atmos fields in InitializeAdvertise and set in ESMF internal state

manually override IOB%fluxes since FMS coupler_type routine doesnt set override flag

update generic_tracer_nuopc.md
revert changes in build.sh

revert changes adding oxygen to datm

tidy MOM cap and add links where code was copied from other sources
move allocation of coupler field work array outside of loop
…nknown to allow flux calculation with overridden pcair fields
@dougiesquire dougiesquire self-assigned this May 2, 2024
@dougiesquire dougiesquire marked this pull request as ready for review May 13, 2024 05:51
@dougiesquire
Copy link
Collaborator Author

I've done some basic tests to show that these changes don't impact performance when no generic tracers are configured - see here.

@micaeljtoliveira I've made a couple of minor changes since your last review. Would you mind taking a quick look and approving if you're happy. I'd like to get this merged.

I'm not sure whether to squash this or not. There's useful info in many of the individual commits. But I made a bit of a mess of the history with the last few commits because the files being patched changed quite a bit and the conflicts were very difficult to resolve (I basically ended up regenerating the patches...).

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.

None yet

4 participants