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

move shortwave absorbed in negligible snow layers to surface #355

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

eclare108213
Copy link
Contributor

@eclare108213 eclare108213 commented Mar 17, 2021

  • Short (1 sentence) summary of your PR:
    Bug fix to handle shortwave absorbed in very thin snow layers that are ignored by the thermo and shortwave routines.
  • Developer(s):
    E. Hunke
  • 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.

Icepack base_suite including regression with current version:
148 measured results of 148 total results
148 of 148 tests PASSED
0 of 148 tests PENDING
0 of 148 tests MISSING data
0 of 148 tests FAILED

I'll try testing this in CICE next.

  • How much do the PR code changes differ from the unmodified code?
    • bit for bit but I expect the results will change in lengthier or more varied tests
    • 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:
    Addresses Negligible snow and convergence #279. Instead of changing the shortwave fluxes only for the thermodynamic convergence criteria, as was done in Negligible snow and convergence #279, this modification moves the absorbed shortwave in thin snow layers to the surface, in the spirit of the redistributed shortwave changes in Shortwave redistribution option added to namelist. #326. However the current redistribution is always done, not just when sw_redist=T, because it fixes a bug in the mushy thermo convergence criterion.
    EDIT for clarification: this change is always done for mushy thermo, but the problem may still exist for other thermodynamic options. That's a science project.

@eclare108213
Copy link
Contributor Author

There's a problem with pushing test results to the wiki from my mac -- "invalid command code T" ?

bash-3.2$ ./report_results.csh
./report_results.csh: Running results.csh
Cloning into 'Test-Results.wiki'...
remote: Enumerating objects: 143, done.
remote: Counting objects: 100% (143/143), done.
remote: Compressing objects: 100% (71/71), done.
remote: Total 19620 (delta 117), reused 98 (delta 72), pack-reused 19477
Receiving objects: 100% (19620/19620), 16.80 MiB | 1013.00 KiB/s, done.
Resolving deltas: 100% (17019/17019), done.
./report_results.csh: writing to Test-Results.wiki/icepack_dev/e8df0ad846.conda.macos.21-03-17.201310.0.md
sed: 1: "Test-Results.wiki/icepa ...": invalid command code T
[master 4aefbf85] update e8df0ad conda
2 files changed, 54 insertions(+)
create mode 100644 icepack_dev/e8df0ad846.conda.macos.21-03-17.201310.0.md
Enumerating objects: 8, done.
Counting objects: 100% (8/8), done.
Delta compression using up to 16 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 1.25 KiB | 1.25 MiB/s, done.
Total 5 (delta 4), reused 0 (delta 0)
remote: Resolving deltas: 100% (4/4), completed with 3 local objects.
To https://github.com/CICE-Consortium/Test-Results.wiki.git
f227cd23..4aefbf85 master -> master

@apcraig
Copy link
Contributor

apcraig commented Mar 18, 2021

sed on mac is different than on linux. we've had problems like this before with mac sed. I'll try to look into it. Ultimately, the script still seems to have worked, https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#e8df0ad8469b46b5ac6b32b4b42e52cd7f872a34, so that's good at least.

@eclare108213
Copy link
Contributor Author

That's weird - it's on the icepack_by_hash_forks page but not on icepack_by_mach_forks page where I looked.

@apcraig
Copy link
Contributor

apcraig commented Mar 18, 2021

You're right, I hadn't noticed that. That's a nice clue to where the problem is.

@eclare108213
Copy link
Contributor Author

eclare108213 commented Mar 18, 2021

CICE base-suite regression tests pass except for the usual 40-processor runs on badger.
https://github.com/CICE-Consortium/Test-Results/wiki/d627ea24f2.badger.intel.21-03-18.003159.0

I do expect this change to be non-BFB, but apparently the case 0 < hs < hs_min does not occur in our test suite.
EDIT: or if it does, then Sswabs=0.

I would like @dabail10 and @MichaelWinton to review this PR, since I've refactored the sw_redist logic and this fix is slightly different from what was done in SIS. Thx

@dabail10
Copy link
Contributor

The more I think about this, this sounds like an inconsistency in hs_min between the radiation and the thermo. Perhaps there could be a case where the snow is barely above hs_min during the radiation computation, but then it disappears before it gets to the thermo? @MichaelWinton, are you using the same hs_min for both?

@eclare108213
Copy link
Contributor Author

The radiation and the thermo are both keyed off of the hs_min parameter in our code (I'm not sure about SIS). My assumption is that there can be Sswabs associated with nonzero hs < hs_min, which might be left over from changes in snow thickness such as the one you suggest, @dabail10. Conservation of heat has never reached roundoff level in CICE, and probably it's at least partly due to leakages associated with these cutoff values not being properly dealt with - that's a bigger project to fix.

@dabail10
Copy link
Contributor

The problem with these type of errors is they are so intermittent. I'm not sure how to test for this. I think this solution is fine. Should we not do it for BL99 as well?

@eclare108213
Copy link
Contributor Author

I originally looked at doing it generally rather than just in mushy thermo, but that's where the sw_redist block was... :). I decided that decision could be made as part of a bigger project to fix the conservation issues.

@eclare108213 eclare108213 marked this pull request as ready for review March 23, 2021 14:57
@apcraig
Copy link
Contributor

apcraig commented Mar 23, 2021

Ready to merge?

@eclare108213 eclare108213 merged commit c027a3c into CICE-Consortium:master Mar 23, 2021
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.

Negligible snow and convergence
4 participants