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

Spear ecda #1453

Merged
merged 20 commits into from
Sep 1, 2021
Merged

Spear ecda #1453

merged 20 commits into from
Sep 1, 2021

Conversation

MJHarrison-GFDL
Copy link
Contributor

These updates are supporting the SPEAR online ensemble data assimilation system using MOM6. This update includes minor changes to existing interfaces, as well as a complete implementation of the online incremental updates returned from the ensemble filter routine. A background temperature and salinity increment (e.g. an ensemble climatology of previous increments) read from file can be enabled using the APPLY_TRACER_TENDENCY_ADJUSTMENT option .

…ncy adjustment to tracers

 - This is a more complete implementation of the online ensemble data assimilation framework
   used in GFDL coupled data assimilation experiments.
 - There are some minor changes to previous DA interfaces.
 - Parameter ASSIM_METHOD ('NONE','OI_ASSIM','EAKF') are the current options.
 - If APPLY_TRACER_TENDENCY_ADJUSTMENT  = True, a time-varying tendency adjustment is provided
   (TENDENCY_ADJUSTMENT_FILE) and applied at each tracer timestep.
 - If USE_BASIN_FILE = True, a basin distribution file (BASIN_FILE) can be read and passed to the
   DA algorithms.
@MJHarrison-GFDL
Copy link
Contributor Author

Comment from @Hallberg-NOAA : Why is oda_CS%h initialized to "Angstrom_m" instead of zero? This will cause the SPEAR data assimilation configuration to fail dimensional consistency tests.

@MJHarrison-GFDL
Copy link
Contributor Author

Added dimensional rescaling for DA thickness initialization.

@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #1453 (bab9c91) into dev/gfdl (ebd0393) will decrease coverage by 0.03%.
The diff coverage is 0.61%.

❗ Current head bab9c91 differs from pull request most recent head 88da4c9. Consider uploading reports for the commit 88da4c9 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl    #1453      +/-   ##
============================================
- Coverage     29.08%   29.04%   -0.04%     
============================================
  Files           236      236              
  Lines         71294    71391      +97     
============================================
  Hits          20736    20736              
- Misses        50558    50655      +97     
Impacted Files Coverage Δ
src/core/MOM.F90 58.60% <0.00%> (-0.17%) ⬇️
src/ocean_data_assim/MOM_oda_driver.F90 0.00% <0.00%> (ø)
config_src/infra/FMS1/MOM_interp_infra.F90 42.85% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebd0393...88da4c9. Read the comment docs.

@marshallward
Copy link
Collaborator

marshallward commented Sep 1, 2021

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

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.

I have examined this code, and overall it seems to be correct, so I am approving this PR.

There is, however, one minor point that we may want to address in a subsequent PR. The description of the new optional arguments correct_leap_year_inconsistency to the two init_extern_field() functions, which are inherited from the underlying FMS code, as "If present and true, then allow for leap year inconsistency" is confusing to me. Does this option fix the problem or allow it to persist? We should clarify these two comments at some point to unambiguously describe what this argument actually does.

@adcroft
Copy link
Collaborator

adcroft commented Sep 1, 2021

There is, however, one minor point that we may want to address in a subsequent PR. The description of the new optional arguments correct_leap_year_inconsistency to the two init_extern_field() functions, which are inherited from the underlying FMS code, as "If present and true, then allow for leap year inconsistency" is confusing to me. Does this option fix the problem or allow it to persist? We should clarify these two comments at some point to unambiguously describe what this argument actually does.

Just for reference, the relevant bit of FMS code is https://github.com/NOAA-GFDL/FMS/blob/main/time_interp/time_interp_external.F90#L590-L594 .

@Hallberg-NOAA
Copy link
Collaborator

There is no description of this correct_leap_year_inconsistency argument in time_interp_external.F90, but there is a description of this argument (repeated identically 3 times) further into the FMS code in time_interp.F90, for example at https://github.com/NOAA-GFDL/FMS/blob/a7b4e6d5dce7853366834203107977d3f4a1f0cd/time_interp/time_interp.F90#L492-L499. However, even after reading this description in the FMS code, I still don't have a clear idea what this argument is intended to do. Is argument this to allow for datasets with a leap year to be used in models that do not use a leap year (by skipping the extra day of input data), the reverse (by duplicating a day of input data), or something even more obscure to do with the difference between the leap years in Gregorian and Julian calendars? If we are adding an optional argument to MOM6 code, its function should be clearly described.

@Hallberg-NOAA
Copy link
Collaborator

With the updated comments, this finally makes sense to me. Thank you @MJHarrison-GFDL !

@marshallward marshallward merged commit f98b76d into mom-ocean:dev/gfdl Sep 1, 2021
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