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

bug fixes for snow grain radius, brine conservation check #415

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

eclare108213
Copy link
Contributor

@eclare108213 eclare108213 commented Nov 25, 2022

  • Short (1 sentence) summary of your PR:
    This PR corrects the magnitude of a parameter controlling wet metamorphosis of snow (answer changing), and fixes a brine volume conservation check (does not affect the solution).
  • Developer(s):
    @eclare108213
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.

160 measured results of 160 total results
156 of 160 tests PASSED
0 of 160 tests PENDING
2 of 160 tests MISSING data
2 of 160 tests FAILED

Both runs with -s snwgrain have different answers, as expected:
FAIL conda_macos_smoke_col_1x1_debug_run1year_snw30percent_snwgrain compare ibased33 different-data
FAIL conda_macos_restart_col_1x1_snwgrain_snwitdrdg compare ibased33 different-data

This was a change in a recent PR, after the baseline was run - not a problem here:
MISS conda_macos_smoke_col_1x1_debug_fsd12_run1year_short compare ibased33 missing-data
MISS conda_macos_restart_col_1x1_fsd12_short compare ibased33 missing-data

  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

Snow parameter: Oleson et al eq 3.68:

dr_e/dt = 10^18 * C1 * f_liq^3 / (4 * pi * r_e^2)
where
C1 = 4.22e-13 and f_liq = mass fraction of liquid in snow.

This PR removes a factor of 100 from f_liq in the current code and changes the constant to S_wet = 10^18 * C1 = 4.22e5.

Snow drainage: this change allows wet metamorphism to function when liquid water associated with the snow scheme is not used directly in melt ponds, i.e. use_smliq_pnd = .false. It also fixes a bug in the computation of drained water mass.

Brine volume: this change is only to the before-and-after calculations used to check brine volume conservation. There is an extra factor of 10 which ought to be removed, if possible, but I'm leaving it in for now.

All bugs were found during the E3SM/Icepack merge process.

Copy link
Contributor

@njeffery njeffery left a comment

Choose a reason for hiding this comment

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

Approved by inspection. Parameter for wet metamorphism is now consistent with Brun 1989.

@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2022

Ran the QC, it fails. The plots suggest the ice is a bit thinner in the new simulation. Images below are baseline, new modifications, difference. I tested with CICE cb58257857d429c4c2d and Icepack main plus changes up to commit #236023f above.
ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcbase

ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcsbb
ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcbase_minus_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcsbb

@eclare108213
Copy link
Contributor Author

Thank you, @apcraig. I would expect the snow grains to be larger and the resulting ice thinner with the parameter change. The magnitude of the difference (generally less than 0.5 m) does not concern me, especially considering how thick the ice is in the southern hemisphere. That's a bit surprising. Out of curiosity, do you have a QC run with just the default parameters, i.e. with snwgrain and snwitdrdg not turned on?

@eclare108213 eclare108213 marked this pull request as ready for review November 29, 2022 13:16
@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2022

The ice thickness of the baseline run (snw options off) is much thinner than either of the runs with snw options on. The two snw runs are closer to each other than to the baseline. I attach figures for the baseline (no snw options), baseline snw configuration, and new snw configuration (i.e. including mods in this PR) as well as one diff, the baseline - new snw configuration.

ice_thickness_cheyenne_intel_qcchk_gx1_144x1_medium_qc_qcchk
ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcbase
ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcsbb
ice_thickness_cheyenne_intel_qcchk_gx1_144x1_medium_qc_qcchk_minus_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcsbb

@eclare108213
Copy link
Contributor Author

Thank you. Very interesting, and these plots are generating lots more questions. I will move this conversation into an issue. In the meantime, I think we should merge this PR to get the bug fixes into main, and then migrate them into the E3SM fork of Icepack. Anyone opposed?

@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2022

One important comment I might add. The snw cases are starting with the "icdefault" option because restart files are not available. The standard case starts with a proper initial condition. Also, I'm not sure whether these plots are means or something else, maybe someone else knows.

@njeffery
Copy link
Contributor

The magnitudes of the changes look reasonable to me.

Copy link
Contributor

@njeffery njeffery left a comment

Choose a reason for hiding this comment

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

Changes to drain snow also look good. These should be BFB.

@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2022

One more data point, ran a QC for the standard run + icdefault. This is with snw options off. 3 images follow, standard run + icdefault, current snw options run + icdefault (required), difference. The runs still fail QC but the differences in the thickness are reduced relative to comparisons with the standard run from a restart file.

ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc qcbaseic

ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcsbb

ice_thickness_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc qcbaseic_minus_cheyenne_intel_smoke_gx1_144x1_icdefault_medium_qc_snwgrain_snwitdrdg qcsbb

@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2022

Will merge this now and create a PR to migrate this to the E3SM-Project branch.

@apcraig apcraig merged commit da5bb87 into CICE-Consortium:main Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants