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

Updates to ODA driver suggested by @Hallberg-NOAA Issue#277 #297

Merged
merged 10 commits into from
Jan 12, 2023

Conversation

MJHarrison-GFDL
Copy link

These changes follow suggestions from @Hallberg-NOAA for clarity in documentation and code readability. These modifications should not impact existing applications (e.g. SPEAR) which are reliant on this code.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #297 (561bdd3) into dev/gfdl (3f57d75) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 561bdd3 differs from pull request most recent head 6984f9a. Consider uploading reports for the commit 6984f9a to get more accurate results

@@            Coverage Diff            @@
##           dev/gfdl     #297   +/-   ##
=========================================
  Coverage     37.10%   37.10%           
=========================================
  Files           263      263           
  Lines         73791    73789    -2     
  Branches      13759    13756    -3     
=========================================
  Hits          27381    27381           
+ Misses        41348    41346    -2     
  Partials       5062     5062           
Impacted Files Coverage Δ
src/core/MOM.F90 51.38% <0.00%> (ø)
src/ocean_data_assim/MOM_oda_driver.F90 0.00% <0.00%> (ø)

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

Copy link
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.

Thank you very much for these updates, @MJHarrison-GFDL. They are a significant improvement to the code!

I have a couple of other minor, specific tweaks to suggest that I think would make this even better and avoid the need for another revision to comply with an imminent change to the optional arguments for the get_param interface for real variables.

The other changes that we might consider as a part of this would be:

  1. Change the units of the dt argument to apply_oda_tracer_increments() from [s] to [T_to_s],
  2. Correct the units of T_bias and S_bias at about line 570 in get_bias_correction_tracer(), including changing the scale factor in the call to horiz_interp_and_extrap_tracer() for temperature from scale=US%degC_to_C to scale=US%degC_to_C*US%s_to_T, as well as the floor on T_bias at about line 592, and similarly for salinity.

@MJHarrison-GFDL
Copy link
Author

Thank you @Hallberg-NOAA . I have addressed your latest comments. I modified the masking in the bias_adjustment routine so that it is based on the masking flag returned from horiz_interp_and_extrap_tracer, rather than the arbitrary ceiling value used in the previous version. This change should not impact answers for MOM6_version>=20190101 .

@Hallberg-NOAA
Copy link
Member

Thanks for the updates to address the overall comments @MJHarrison-GFDL , but the specific comments relating to the get_param calls also need to be dealt with. As written, this code will no longer compile once PR #300 is merged into dev/gfdl.

@marshallward
Copy link
Member

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

@marshallward marshallward merged commit 1d918b6 into NOAA-GFDL:dev/gfdl Jan 12, 2023
@MJHarrison-GFDL MJHarrison-GFDL deleted the DA_cleanup branch April 10, 2023 16:54
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.

Dimesionally incorrect expressions in apply_oda_tracer_increment()?
3 participants