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

Shortwave redistribution option added to namelist. #326

Merged
merged 17 commits into from
Jul 15, 2020

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented Jul 7, 2020

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:
    We need the "shortwave redistribution" option for ktherm=2 for CESM. I believe it was needed for E3SM as well. So, I have added it here, but with an option to turn
    it off via the namelist.
  • 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_mach_forks#izumi
  • 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 is bfb with sw_redist=.true. and ktherm=1 and sw_redist = .false. and ktherm=2. Since the default for sw_redist will be .false. we should warn people who use the BL99 thermo (ktherm=1) that this will now be off and they will need to turn it on if necessary for their runs.

@apcraig
Copy link
Contributor

apcraig commented Jul 7, 2020

Looks like some hardwired parameters were moved to namelist and the results are bit-for-bit. Why are the alt03, alt04, and thermo1 test cases bit-for-bit with the code changes?

@dabail10
Copy link
Contributor Author

dabail10 commented Jul 7, 2020

For those tests, I added sw_redist = .true.

@dabail10
Copy link
Contributor Author

dabail10 commented Jul 7, 2020

This also helps get rid of some CESMCOUPLED ifdefs.

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.

This looks okay to me, especially since it can be turned off. It's a little confusing that the default was for it to be on for BL99, and unavailable (i.e. off) for mushy, so it's not possible to be entirely consistent with the previous code.
Since this affects the mushy thermo, I'm alerting @proteanplanet and @akturner for feedback.

doc/source/science_guide/sg_thermo.rst Outdated Show resolved Hide resolved
@apcraig
Copy link
Contributor

apcraig commented Jul 7, 2020

For those tests, I added sw_redist = .true.

I'm still a little confused. I understand that in alt03, alt04, and thermo1, sw_redist is now true. What I don't understand is why everything is bit-for-bit with the prior results when sw_redist is true for some cases and false for other cases. Was another flag (which one?) playing the same role as sw_redist prior to this change?

As a followup, subroutine temperature_changes_salinity has some new code that is on when sw_redist is true. Is this just diagnostic?

@dabail10
Copy link
Contributor Author

dabail10 commented Jul 7, 2020

The old code was only present in ice_therm_bl99.F90 (ktherm=1). It was not present in ice_therm_mushy.F90 (ktherm=2). So, it was only effectively "true" for the ktherm=1 case.

Copy link
Contributor

@proteanplanet proteanplanet left a comment

Choose a reason for hiding this comment

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

This is an important addition.

@apcraig
Copy link
Contributor

apcraig commented Jul 15, 2020

I think all comments have been addressed and this is ready to merge.

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.

4 participants