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

+wave_speed arg mono_N2_depth in thickness units #424

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

Hallberg-NOAA
Copy link
Member

Changed the units of the optional mono_N2_depth argument to wave_speed, wave_speed_init and wave_speed_set_param in thickness units instead of height units. Accordingly, the units of one element each in the diagnostics_CS and wave_speed_CS and a local variable in VarMix_init are also changed to thickness units. The unit descriptions of some comments describing diagnostics were also amended to also describe the non-Boussinesq versions. Because this is essentially just changing when the unit conversion occurs, all answers are bitwise identical, but there are changes to the units of an optional argument in 3 publicly visible routines.

@Hallberg-NOAA Hallberg-NOAA added the refactor Code cleanup with no changes in functionality or results label Jul 24, 2023
  Changed the units of the optional mono_N2_depth argument to wave_speed,
wave_speed_init and wave_speed_set_param in thickness units instead of height
units.  Accordingly, the units of one element each in the diagnostics_CS and
wave_speed_CS and a local variable in VarMix_init are also changed to thickness
units.  The unit descriptions of some comments describing diagnostics were also
amended to also describe the non-Boussinesq versions.  Because this is
essentially just changing when the unit conversion occurs, all answers are
bitwise identical, but there are changes to the units of an optional argument in
3 publicly visible routines.
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #424 (b689912) into dev/gfdl (636d610) will decrease coverage by 0.01%.
The diff coverage is 23.07%.

❗ Current head b689912 differs from pull request most recent head 1943a9f. Consider uploading reports for the commit 1943a9f to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #424      +/-   ##
============================================
- Coverage     38.15%   38.15%   -0.01%     
============================================
  Files           269      269              
  Lines         76737    76742       +5     
  Branches      14119    14122       +3     
============================================
  Hits          29278    29278              
- Misses        42184    42189       +5     
  Partials       5275     5275              
Files Changed Coverage Δ
...eterizations/lateral/MOM_lateral_mixing_coeffs.F90 42.64% <0.00%> (ø)
src/diagnostics/MOM_wave_speed.F90 37.51% <18.18%> (+0.01%) ⬆️
src/diagnostics/MOM_diagnostics.F90 71.52% <100.00%> (ø)

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

@raphaeldussin raphaeldussin left a comment

Choose a reason for hiding this comment

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

I mostly agree with the proposed changes. I think some variables may need to be changes to thickness units.

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

On behalf of @raphaeldussin

@marshallward
Copy link
Member

marshallward commented Jul 25, 2023

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

@marshallward marshallward merged commit 878fd1e into NOAA-GFDL:dev/gfdl Jul 26, 2023
@Hallberg-NOAA Hallberg-NOAA deleted the mono_N2_depth_units branch July 26, 2023 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code cleanup with no changes in functionality or results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants