Skip to content

Add save_MOM6_internal_state subroutine to MOM.F90 and call during restart#375

Merged
marshallward merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
cspencerjones:MOM-internal-restart
Jul 12, 2023
Merged

Add save_MOM6_internal_state subroutine to MOM.F90 and call during restart#375
marshallward merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
cspencerjones:MOM-internal-restart

Conversation

@cspencerjones
Copy link
Copy Markdown

As described in issue #372, I would like to be able to create restart files that contain information about the particle location. These files should be written at the same time as other restart files. I cannot add these calls directly to the driver, because the driver does not have information about the particle location.

We have opted to add save_MOM6_internal_state as a subroutine in MOM.F90, and to call it from each of the drivers. We hope this will allow for more new packages to write restart files in the future.

I have tested these additions for solo_driver and FMS_cap, but I don't have access to the other drivers, so I haven't been able to test them.

cc @adcroft

@cspencerjones cspencerjones marked this pull request as draft June 2, 2023 20:27
@marshallward marshallward requested a review from adcroft June 3, 2023 17:00
@cspencerjones cspencerjones force-pushed the MOM-internal-restart branch from 0e5dc7b to 631342b Compare June 9, 2023 15:57
@cspencerjones
Copy link
Copy Markdown
Author

Since Alistair is one of the authors of this PR, it probably doesn't make sense for him to be a reviewer. Can you assign someone else @marshallward ?

@cspencerjones cspencerjones marked this pull request as ready for review June 23, 2023 20:39
@marshallward
Copy link
Copy Markdown
Member

I can review it (hopefully very soon).

@marshallward marshallward requested review from marshallward and removed request for adcroft June 23, 2023 21:14
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 23, 2023

Codecov Report

Merging #375 (70f33aa) into dev/gfdl (77b5881) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head 70f33aa differs from pull request most recent head a7e4a6f. Consider uploading reports for the commit a7e4a6f to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #375      +/-   ##
============================================
- Coverage     38.22%   38.22%   -0.01%     
============================================
  Files           269      269              
  Lines         76413    76417       +4     
  Branches      14031    14030       -1     
============================================
- Hits          29209    29208       -1     
- Misses        41962    41967       +5     
  Partials       5242     5242              
Impacted Files Coverage Δ
config_src/drivers/solo_driver/MOM_driver.F90 50.82% <33.33%> (-0.22%) ⬇️
src/core/MOM.F90 51.45% <66.66%> (+0.09%) ⬆️

... and 1 file with indirect coverage changes

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

Copy link
Copy Markdown
Member

@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 seems to me like a reasonable solution to the problem with calling the routines to save the restart files for submodules that are known to MOM6 but not to the driver, as discussed in the description of this PR. As noted in the comments in this commit, there is more work to be done along these lines, but this is a viable way-point in the code changes. This can be merged into dev/gfdl in due course, once the pipeline regression testing has passed.

As described in issue mom-ocean#372, I would like to be able to create restart files that
contain information about the particle location. These files will be written at
the same time as other restart files. I cannot add these calls directly to the
driver, because the driver does not have information about the particle location.
We have added save_MOM6_internal_state as a subroutine in MOM.F90, and
we added calls to this subroutine from each of the drivers. We hope this will
allow for more new packages to write restart files in the future.

Co-authored by Spencer Jones <spencerjones@tamu.edu>
@Hallberg-NOAA Hallberg-NOAA force-pushed the MOM-internal-restart branch from bb924a7 to 08c3a17 Compare June 27, 2023 10:39
@marshallward
Copy link
Copy Markdown
Member

Given that this changes the drivers, which needs consultation with the partners, should we just push ahead with the suggested TODO plan and introduce save_MOM_restart which calls save_restart?

I can work on this and submit it as a PR to this PR.

@cspencerjones
Copy link
Copy Markdown
Author

I'm happy for you to do that @marshallward , as long as it doesn't take months and months.

@marshallward
Copy link
Copy Markdown
Member

I've submitted a potential revision to this PR which does the following:

  • merges save_restart and save_MOM6_internal_state into a generalized restart function.

  • Removes MOM_restart dependencies from the primary model driver code. (It is still present in the surface forcing modules).

The PR is here for review: cspencerjones#1

It's potentially disruptive, so I expect it needs careful review before merging. If it's too much, then maybe we can leave it for a later date.

Copy link
Copy Markdown
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

We can take this as-is, and the suggested amendments will be handled in a separate PR.

@marshallward
Copy link
Copy Markdown
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/19805 ✔️

@marshallward marshallward merged commit 7970347 into NOAA-GFDL:dev/gfdl Jul 12, 2023
@cspencerjones
Copy link
Copy Markdown
Author

Thanks very much @marshallward !!

dhruvbalwada pushed a commit to dhruvbalwada/MOM6 that referenced this pull request Jan 31, 2026
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.

4 participants