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

Fix to dumbbell initialization in layer mode #160

Merged
merged 10 commits into from
Jul 18, 2022

Conversation

WenhaoChen89
Copy link

This PR fixes the initialization of the dumbbell test to run with the layer mode.

  1. The initial layer thickness is changed to follow the isopycnal
  2. The initial salinity is set up using the 'fit' method.
  3. The sponge is added for the layer mode

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #160 (6909c58) into dev/gfdl (05e705d) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 6909c58 differs from pull request most recent head 55b9eda. Consider uploading reports for the commit 55b9eda to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #160      +/-   ##
============================================
- Coverage     34.01%   33.99%   -0.02%     
============================================
  Files           259      259              
  Lines         70218    70248      +30     
  Branches      13011    13016       +5     
============================================
  Hits          23884    23884              
- Misses        41835    41865      +30     
  Partials       4499     4499              
Impacted Files Coverage Δ
src/initialization/MOM_state_initialization.F90 17.23% <ø> (ø)
src/user/dumbbell_initialization.F90 0.00% <0.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 05e705d...55b9eda. Read the comment docs.

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.

Overall this is a useful contribution that is moving in the right direction, but there is a dimensionally incorrect expression (noted in the line-specific comments) that will need to be corrected before this PR can be accept. As written, this would fail the dimensional consistency testing if Z_RESCALE_POWER is not set equal to H_RESCALE_POWER.

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.

Some of the issues with this code have now been addressed, but the dimensionally inconsistent expression on line 454 of dumbbell_initialization.F90 still needs to be corrected by multiplying h_in by GV%H_to_Z. I would also request that after this change is made, that this contribution should be tested for dimensional consistency by running it with various settings for H_RESCALE_POWER and Z_RESCALE_POWER; the answers should be identical for any values between about -140 and 140 for these factors. For completeness of testing these two parameters should NOT use the same value.

@Hallberg-NOAA
Copy link
Member

All of my concerns with this PR have now been addressed, and I see this as a valuable new contribution.

I am, however, recommending that we handle this PR via a squash merge, as it has a relatively high number of commits (9) for a straightforward new contribution, and several of the commits are partial or full reversions of the earlier commits.

@marshallward
Copy link
Member

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

(NOTE: There were multiple errors from the CI, but appear to be network-related and not to do with this PR.)

@marshallward marshallward merged commit 64fe4fc into NOAA-GFDL:dev/gfdl Jul 18, 2022
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.

3 participants