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

Use ACCESS-NRI fork of generic tracers with access couplers #27

Merged
merged 7 commits into from
Aug 23, 2024

Conversation

dougiesquire
Copy link

@dougiesquire dougiesquire commented Aug 7, 2024

Retry of #26, now from a branch on this repo.

See mom-ocean#388 for context and details.

This PR was originally made to mom-ocean/MOM5 but it was decided that we would make the changes here first while some details about the build system and testing are still being resolved. Comments from @aidanheerdegen's review of the original PR have been incorporated here.

Note, this PR includes the ACCESS-NRI fork of generic_tracers as a git submodule.

Note also, this PR removes the need for the ACCESS-OM-BGC build type (and hence the CSIRO_BGC pp def). The ACCESS-OM build type now always builds with the USE_OCEAN_BGC pp def and generic tracer WOMBAT can be configured at runtime. I’ve verified that always building with USE_OCEAN_BGC doesn’t impact performance when generic tracers are not configured. To do this, I ran two ACCESS-OM2 experiments on the NCI Gadi supercomputer:

  • Exp 1 uses this configuration modified to run for 1 year at a time and to use a MOM5 executable built from 59e0aad (the most recent commit on main at the time of opening this PR).
  • Exp 2 uses the same configuration, but the MOM5 executable used includes the changes in this PR.

I ran three years of each. The experiments were run at the same time. The chksum outputs in the logs are identical between the experiments and the walltimes reported by PBS for each year are given in the table below.

  Exp 1 Exp 2
year 1 959 s 908 s
year 2 1031 s 1002 s
year 3 1148 s 1028 s
average 1046 s 979 s

Note also also, I've hijacked the ACCESS-ESM build type to build with the ACCESS-NRI fork of generic tracers and with coupling support for the new WOMBATlite. This changes the behaviour of this build type, which used to build with the old MOM5 WOMBAT tracer package. However, we haven't been using this build type to build ACCESS-ESM1.5 anyway - instead ACCESS-ESM1.5 is built with the ACCESS-CM build type using the access-esm1.5 branch. With these changes, we should be able to use the master branch for ACCESS-ESM1.6.

Closes mom-ocean#388

@dougiesquire
Copy link
Author

@CodeGat, could you please do the workaround for ACCESS-NRI/build-ci#170?

@dougiesquire
Copy link
Author

I'm marking this as a draft because I'm going to see if I can extend this for ACCESS-ESM also.

@dougiesquire dougiesquire marked this pull request as draft August 8, 2024 02:10
@CodeGat
Copy link

CodeGat commented Aug 8, 2024

@CodeGat, could you please do the workaround for ACCESS-NRI/build-ci#170?

Done @dougiesquire

@dougiesquire dougiesquire force-pushed the dougiesquire/issue388-accessom-gtracer branch from 7fe7ec8 to c26ae75 Compare August 15, 2024 22:31
@dougiesquire dougiesquire changed the title Use ACCESS-NRI fork of generic tracers with accessom_coupler Use ACCESS-NRI fork of generic tracers with access couplers Aug 15, 2024
@dougiesquire dougiesquire marked this pull request as ready for review August 15, 2024 22:53
@dougiesquire dougiesquire force-pushed the dougiesquire/issue388-accessom-gtracer branch 2 times, most recently from 0b98bd0 to 7bb80a9 Compare August 16, 2024 06:44
@dougiesquire
Copy link
Author

dougiesquire commented Aug 17, 2024

Note this PR really requires the changes in mom-ocean#390 for WOMBATlite to run correctly when there is surface salt restoring/correction (as in ACCESS-OM2). Without mom-ocean#390, things will still run, but no virtual flux correction will be applied based on the salt flux added.

Ideally, both mom-ocean#390 and this PR would be in mom-ocean/MOM5, but given how difficult it is to find suitable reviewers we may end up also moving mom-ocean#390 over to this ACCESS-NRI/MOM5.

UPDATE: I've now cherry-picked the changes in mom-ocean#390 into this PR.

@dougiesquire
Copy link
Author

Note also, I think the build CI here only builds the ACCESS-OM build type, so the modifications to the ACCESS-ESM build type is this PR are not being tested in the CI.

@dougiesquire dougiesquire force-pushed the dougiesquire/issue388-accessom-gtracer branch from 3f3a2e2 to 53e9301 Compare August 20, 2024 09:11
@dougiesquire
Copy link
Author

dougiesquire commented Aug 20, 2024

Okay, this PR now includes the complete set of changes required for ACCESS-OM2 and ACCESS-ESM1.5/1.6 with generic tracer WOMBATlite. In addition to the necessary changes to the accessom/cm_coupler driver code, I've also had to:

  • Add the calculation of opacity to the ocean_shortwave_csiro module (58419f4). The old WOMBAT didn't use this, but the generic tracer one does.
  • Allow passing salt flux restoring/correction to generic tracers (b1f2a5f). I originally proposed this change to mom-ocean/MOM5 here, but haven't been able to find a reviewer there so I've cherry-picked it into this PR.

@aidanheerdegen, this is ready for review. I've done test runs of both ACCESS-OM2 and ACCESS-ESM1.5 using the changes in this PR and things look sensible afaict.

@dougiesquire
Copy link
Author

dougiesquire commented Aug 21, 2024

Note there are prerelease deployments of ACCESS-OM2 and ACCESS-ESM1.6 built from this PR:

aidanheerdegen
aidanheerdegen previously approved these changes Aug 22, 2024
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

I made some comments and pointed out a couple of old historical typos, but I'm really not familiar enough with the coupling stuff to know if you've missed anything. I think we're relying on the fact that it all seems to work to check that.

I assume fields still conserve and fluxes seem reasonable?

src/access/accesscm_coupler/mom_oasis3_interface.F90 Outdated Show resolved Hide resolved
src/access/accesscm_coupler/ocean_solo.F90 Outdated Show resolved Hide resolved
src/access/shared/gtracer_flux.F90 Show resolved Hide resolved
src/mom5/ocean_bgc/ocean_generic_tracer.F90 Outdated Show resolved Hide resolved
src/mom5/ocean_bgc/ocean_generic_tracer.F90 Show resolved Hide resolved
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

LGTM.

@dougiesquire
Copy link
Author

Thanks @aidanheerdegen, your eyes are very much appreciated. You have lovely eyes.

@dougiesquire dougiesquire merged commit 6d87686 into master Aug 23, 2024
4 checks passed
@dougiesquire dougiesquire deleted the dougiesquire/issue388-accessom-gtracer branch August 23, 2024 05:50
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.

Allow generic tracers with accessom coupler
3 participants