dEdd aerosol fixes #400
dEdd aerosol fixes #400
Conversation
- update g/w0/tau calculation, get rid of use of puny - add missing g/w0/tau calculation after "aerosol in snow" calculation - fix bug in computation of modal_aero taer/waer/gaer, changed divide to multiple of rnilyr/rnslyr - refactor divides of rnilyr/rnslyr to multiplies of real(nilyr/nslyr) Generally changes answers for tr_aero
|
This is a draft because we should discuss whether these changes are correct and whether this is what we want to do. Otherwise, I think these changes are working. |
|
I have to test this out in CESM. Dave |
There was a problem hiding this comment.
This all looks correct to me. Since standard testing is BFB, the compiler/optimizer must be fixing the 1/(1/n)=n coding issues. Testing with tr_aero=T in a realistic simulation would be helpful, as a sanity check -- I would not expect those results to be BFB -- although these bugs should be fixed, regardless. Thanks much.
| tau(k) = tau(k) + taer | ||
| enddo ! k | ||
| endif ! tr_aero | ||
|
|
||
| ! pond | ||
| else !if( srftyp == 2 ) then | ||
| ! pond water layers evenly spaced | ||
| dz = hp/(c1/rnslyr+c1) | ||
| dz = hp/(real(nslyr,kind=dbl_kind)+c1) |
There was a problem hiding this comment.
I think this change is correct, but I question the original expression -- does anyone understand why the +1 is in the denominator? It looks like the dEdd scheme is dividing the pond depth (volume) evenly among the snow layers, but then why is this not dz=hp/nslyr ?
There was a problem hiding this comment.
I believe this was the "surface scattering layer" versus the "internal layer". So, I think this is fine.
There was a problem hiding this comment.
Okay, now I see that the loop immediately following this line is over k = 0, nslyr. So the ponds are simply divided into nslyr+1 equally-spaced layers for the dEdd calculations, and I won't worry about what "SSL" might mean for a pond -- it's just bookkeeping for consistency with the snow calculation, I guess. Thanks.
| (w0(k)*tau(k) + waer) | ||
| w0(k) = (w0(k)*tau(k) + waer) / & | ||
| (tau(k) + taer) | ||
| tau(k) = tau(k) + taer |
There was a problem hiding this comment.
It does look like these three lines will impact tr_aero and model_aero = .false. I looked back in the CICE5 code and this was the original implementation. Trying it out now.
There was a problem hiding this comment.
The changes to the "puny" implementation elsewhere will change answers but only slightly. On the other hand, this block of changes was a bug and could be more consequential.
| waer_tab(ns,(1+(na-1)/4)) | ||
| gaer = gaer + & | ||
| (aero_mp(na+1)/rnslyr)*kaer_tab(ns,(1+(na-1)/4))* & | ||
| (aero_mp(na+1)*rnslyr)*kaer_tab(ns,(1+(na-1)/4))* & |
There was a problem hiding this comment.
This is almost comical ...
There was a problem hiding this comment.
I guess this is just modal aerosol. Not sure if you or others are using that. We haven't been testing it in CICE because it hasn't been working. But now it does and I have added tests to both Icepack and CICE. Still can't say whether answers are scientifically correct though.
| g(k) = (g(k)*w0(k)*tau(k) + gaer) / & | ||
| (w0(k)*tau(k) + waer) | ||
| w0(k) = (w0(k)*tau(k) + waer) / & | ||
| (tau(k) + taer) |
There was a problem hiding this comment.
This also looks like it might impact answers.
There was a problem hiding this comment.
These changes should be round-off-ish.
|
After four years, the answers are definitely different. I want to run it longer to see if it is "climate" changing. |
|
I wouldn't be surprised if these mods are "climate changing" when tr_aero=T and modal_aero=F, when comparing CICE6 before/after this PR. Is that what you're testing @dabail10? These are bugs in CICE6 that were not present in CICE5, and should be fixed regardless of whether they are deemed to be climate changing, right away. If this modified code produces reasonable-looking answers, then I think we should merge it and keep moving forward. More careful comparisons with CICE5 might be needed to understand differences between the code versions, but let's at least get the obvious bugs fixed ASAP. We may need to make further modifications later, as we continue the BGC merge. Is this plan okay with those of you reading this thread? |
|
@eclare108213, I agree that these things need to be fixed. We should try to understand the impact of the bug and document that, but that could be done over days/weeks as needed. |
|
@dabail10 How are your runs going? I'd like to merge this one. |
|
I have 20 years of both. Here are some diagnostics: Basically I can't really tell as it is only 20 years, but it does not appear to be climate changing. I will run these out for 100 years. Dave |
to implementation consistent with E3SM Icepack snicar upgrades This change is bit-for-bit for the full test suite on cheyenne.
|
Can we merge while we continue to assess? |
|
I would say go ahead. |
PR checklist
Short (1 sentence) summary of your PR:
dEdd aerosol fixes
Developer(s):
apcraig, 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.
bit-for-bit against current Icepack trunk, https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#0717620693e6545c0c693cb17987a5e412162b79. Also testing CICE suite on cheyenne.
How much do the PR code changes differ from the unmodified code?
Does this PR create or have dependencies on CICE or any other models?
Does this PR add any new test cases?
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/.)
Please provide any additional information or relevant details below:
update g/w0/tau calculation, get rid of use of puny
add missing g/w0/tau calculation after "aerosol in snow" calculation
fix bug in computation of modal_aero taer/waer/gaer, changed divide to multiple of rnilyr/rnslyr
refactor divides of rnilyr/rnslyr to multiplies of real(nilyr/nslyr)
add modal option, update test suite