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

Add salt flux computation option #392

Closed
wants to merge 0 commits into from

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented Jun 16, 2022

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

  • Short (1 sentence) summary of your PR:
    This adds the option to use the prognostic salinity in the salt flux computation.
  • Developer(s):
    dabail10 (D. Bailey)
  • 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.
    https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#7c33d8ebeaa42d3d76767b7825730a9c2a555ec2
  • 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:
    This adds saltflux_option to the namelist for icepack. The options are '4psu' or 'prognostic'. I also removed the if (ktherm == 1) condition around the code for update_ocn_f in ice_therm_itd.F90 (add_new_ice). We need to compute these fluxes regardless of the ktherm option. I'm not sure where in MPAS-SeaIce the salt and fresh water fluxes are computed. Can we use update_ocn_f here? Related to issue update_ocn_f problem #390 Added test results comparing to cheyenne baselines.

@njeffery
Copy link
Contributor

@dabail10 : Should saltflux_option be added to Icepack_in at this point?

@dabail10
Copy link
Contributor Author

Yes! Good catch.

@dabail10
Copy link
Contributor Author

Technically, it should also be added to the Icepack driver as well.

@njeffery
Copy link
Contributor

Okay, I'll wait for that.

Also, do any of the Icepack testcases verify salt conservation?

@dabail10
Copy link
Contributor Author

Unfortunately, there are no conservation diagnostics in Icepack. Something on my to-do list.

@dabail10
Copy link
Contributor Author

We do check salt conservation in CICE.

@njeffery
Copy link
Contributor

I noticed that there's a conserv_check option. Would I see additional diagnostics in the log with this true?

@dabail10
Copy link
Contributor Author

Good question. I've never turned that on.

@eclare108213
Copy link
Contributor

I think conserv_check is for checking algorithms like incremental remapping or ridging -- it used to be hardwired in the code for debugging during development, and was later moved out to the namelist for easier automated testing.

write (c_hinmax1, '(f7.3)') hin_max(n-1)
write (c_hinmax2, '(f7.3)') hin_max(n)
write (c_hinmax1, '(f6.3)') hin_max(n-1)
write (c_hinmax2, '(f6.3)') hin_max(n)
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the format here changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. Not sure. I did not mean to change this.

@@ -359,7 +359,8 @@ forcing_nml
"", "``mm_per_sec``", "(same as MKS units)", ""
"", "``m_per_sec``", "", ""
"``restore_ocn``", "logical", "restore sst to data", "``.false.``"
"``tfrz_option``", "``linear_salt``", "linear functino of salinity (ktherm=1)", "``mushy``"
"``saltflux_option``", "``4psu``","``prognostic``"
Copy link
Contributor

Choose a reason for hiding this comment

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

The syntax here is not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@apcraig
Copy link
Contributor

apcraig commented Jun 17, 2022

Just want to confirm everything is bit-for-bit in Icepack and CICE? Do you have a link to any test results?

@njeffery
Copy link
Contributor

@dabail10 : Removing ktherm <=1 for update_ocn_f = true would be a problem for the MPAS-seaice implementation. This is because the freshwater and salt fluxes from the freeze melt potential are computed originally by the ocean model and corrected to be consistent with mushy-layer in the driver, ice_comp_mct. If the correction is done in add_new_ice instead of the driver then it should look like the update_ocn_f = false version dfresh = -rhoi*(vi0new-vi0tmp)/dt , not dfresh = -rhoi*vi0new/dt.

@njeffery
Copy link
Contributor

Maybe a flag that specified (ktherm == 1) or (ktherm ==2 and saltflux_option == prognostic) would work

@dabail10
Copy link
Contributor Author

My understanding of your use case is that update_ocn_f would be .false.? That's why we only apply the correction when update_ocn_f is .false.

@njeffery
Copy link
Contributor

@dabail10 : Do you know of any other code that's impacted by update_ocn_f? Right now E3SM runs with update_ocn_f = true and switching to false would break mass/salt conservation without some code change.

@dabail10
Copy link
Contributor Author

I guess I am not understanding correctly. I'm not sure about other coupled models using update_ocn_f, maybe NOAA ... my understanding is that update_ocn_f = .true. means that the sea ice component computes the fluxes of freshwater and salt due to frazil formation. The ocean component computes it when update_ocn_f = .false. and then the sea ice component sends a correction for the "extra" frazil. The rationale for this is that the sea ice component would compute the freshwater and salt and use the ktherm == 2 actual salinity.

@njeffery
Copy link
Contributor

That all makes sense to me. I'll look into why it's done differently in MPAS-seaice and make sure update_ocn_f hasn't taken on additional meaning.

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

The logic associated with update_ocn_f, ktherm, and now saltflux_option is really convoluted. Adding this new option only makes it worse. Can we work out what the logic ought to be and fix this?

Mushy handles the flux calculations quite differently from the other thermodynamics options. Can the original flags be pushed down into the mushy routines and take care of things there?

It seems that saltflux_option='prognostic' only when ktherm=2 (mushy), but ktherm=2 does not imply that saltflux_option='prognostic' (it can be constant, e.g. 4 psu). So there are three separate issues, partly related and all interacting: 1) whether the ocean assumes that frazil ice is included in the water and salt fluxes, controlled by update_ocn_f, 2) whether the salinity coupling is via an assumed constant value or calculated prognostically by the thermodynamics, controlled by saltflux_option, and 3) whether the ice salinity is prognostic or not, controlled by ktherm. There needs to be a check (in CICE too!) that if saltflux_option='prognostic' then ktherm=2, otherwise abort.

Am I understanding this correctly?

@@ -241,7 +242,7 @@ subroutine thermo_vertical (nilyr, nslyr, &
einter ! intermediate energy

real (kind=dbl_kind) :: &
fadvocn ! advective heat flux to ocean
fadvocn, saltvol, dfsalt ! advective heat flux to ocean
Copy link
Contributor

Choose a reason for hiding this comment

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

Put the new variables on separate lines so that they can be identified with ! comments (add units too please)

@@ -162,6 +162,11 @@ module icepack_parameters
! 'linear_salt' = -depressT * sss
! 'mushy' conforms with ktherm=2

character(len=char_len), public :: &
saltflux_option = '4psu'! Salt flux computation
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to add a separate parameter for the reference value itself, rather than hardwiring it for 4 psu. What if some other model prefers 4.5 psu? You could use prognostic and constant as the types for now, and then maybe someone would add some other type in the future (seasonal? stochastic?).

Also, "saltflux_option" sounds clunky to me, maybe because it's not immediately clear what kind of "option" it is. We use "type" in other flags, e.g. bgc_flux_type. The equivalent here would work: salt_flux_type. (And yes I realize that there's a tfrz_option, so this revision is optional...)

@dabail10
Copy link
Contributor Author

Thanks for your feedback @eclare108213 ! I was following the example of tfrz_option as you mention. Here we do minus1p8 instead of constant. I agree that it makes more sense to have a constant option for both and let the user assign the value. The issue with saltflux_option is that it is needed in the frazil ice (add_new_ice) in icepack_therm_itd.F90. It is also needed in the zap routines. The way I have it set up now is that update_ocn_f and saltflux_option are independent. You could imagine someone wants to actually do a prognostic salinity flux whether or not the ocean component is doing the freshwater and salt for frazil or not. Also, I would imagine that you could even want a prognostic salinity flux with ktherm == 1 or maybe a different constant value. I thought about doing something like only computing the prognostic salt flux when ktherm == 2.

@dabail10 dabail10 closed this Oct 4, 2022
@dabail10 dabail10 deleted the saltflux_option branch October 4, 2022 17:40
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