ice_dyn_vp: express rheology in terms of bulk and shear viscosities#647
Conversation
|
Small suggestion, I would re-title this PR so that it describes the code changes themselves, something like: |
JFLemieux73
changed the title
| divune, divunw, divusw, divuse , & ! divergence | ||
| tensionne, tensionnw, tensionsw, tensionse, & ! tension | ||
| shearne, shearnw, shearsw, shearse , & ! shearing | ||
| Deltane, Deltanw, Deltasw, Deltase , & ! Delt | ||
| zetax2ne, zetax2nw, zetax2sw, zetax2se , & ! 2 x zeta (visc coeff) | ||
| etax2ne, etax2nw, etax2sw, etax2se , & ! 2 x eta (visc coeff) | ||
| rep_prsne, rep_prsnw, rep_prssw, rep_prsse, & ! replacement pressure |
There was a problem hiding this comment.
These changes to the argument declaration are not strictly needed and add noise to the diff, personally I would drop them.
There was a problem hiding this comment.
Same thing for the arguments to viscous_coeffs_and_rep_pressure below
| !----------------------------------------------------------------- | ||
|
|
||
| str(:,:,:) = c0 | ||
| capping = .true. ! to be improved |
There was a problem hiding this comment.
Do we want to keep that comment ? as it stands I find it a little confusing, i.e. it could mean either "the fact that capping is hardcoded here could be improved" or "it would be an improvement to allow capping = .false. here"...
There was a problem hiding this comment.
replaced by: ! could be later included in ice_in
| call formDiag_step1 (nx_block , ny_block , & | ||
| icellu (iblk) , & | ||
| indxui (:,iblk) , indxuj(:,iblk), & | ||
| dxt (:,:,iblk) , dyt (:,:,iblk), & | ||
| dxhy (:,:,iblk) , dyhx(:,:,iblk), & | ||
| cxp (:,:,iblk) , cyp (:,:,iblk), & | ||
| cxm (:,:,iblk) , cym (:,:,iblk), & | ||
| zetax2 (:,:,iblk,:), etax2(:,:,iblk,:), & | ||
| diag_rheo(:,:,:)) |
There was a problem hiding this comment.
Can we avoid changing the whitespace in these lines ? it also adds noise to the diff so it's harder to spot what is really changing...
| subroutine viscous_coeffs_and_rep_pressure (strength, tinyarea, & | ||
| Deltane, Deltanw, & | ||
| Deltase, Deltasw, & | ||
| Deltasw, Deltase, & | ||
| zetax2ne, zetax2nw, & | ||
| zetax2se, zetax2sw, & | ||
| zetax2sw, zetax2se, & | ||
| etax2ne, etax2nw, & | ||
| etax2se, etax2sw, & | ||
| etax2sw, etax2se, & | ||
| rep_prsne, rep_prsnw,& | ||
| rep_prsse, rep_prssw,& | ||
| rep_prssw, rep_prsse,& |
There was a problem hiding this comment.
In fact, I'm kind of on the fence about changing the order in the interface also. What's the justification ? it because other subroutines use this order? which ones?
apcraig
left a comment
apcraig
left a comment
There was a problem hiding this comment.
This looks fine to me. If you'd like me to run the full suite on 3 compilers on cheyenne, I can do that. Given this only affects a few tests (vp) and that the non-vp cases are working and bit-for-bit in testing thus far, I think it's all good.
|
Please go ahead for the test on cheyenne. Thanks! |
|
I will merge when GHActions are done. I don't think we need to be that concerned with the non bit-for-bit results. It's not surprising to me that refactoring the code introduced a roundoff level change somewhere. The greater concern was the confusion between cmp and ncdiff, but we understand that now. If you feel the non bit-for-bit is unexpected, then feel free to pursue further. But given the results are scientifically identical, that it's just a diagnostic quantity in this implementation that is non bit-for-bit, and that it can be explained by the code changes, I see no reason to do so. |
|
OK, I'll test on cheyenne today then we can merge once we confirm those results. |
|
Testing on cheyenne looks good, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#6f25d10df03158a9f03f02ea4f08fa0d83b59f8f. The dynpicard changes answers as we expect. There is also something funny going on with the regression test of the gx1prod 10 year run. I've diffed a few log files and there are some diagnostics that are different but the science result seems to be identical. I need to look into that more as a separate task. I'll merge now. |
3 participants
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
Modified implicit solver code for consistency with viscous coeff and rep_pressure as used in EVP.
@JFLemieux73
see issue Make implicit solver code more consistent with recent changes to rheology #646
*** as described in the issue, all the regression tests pass except the one for the implicit solver. However, ncdiff of the restart files show they are the same. Furthermore, I ran a five year QC tests (I just changed kdyn from 1 to 3) and the results are BFB. This is a bit contradictory but I would say that to the worst there are roundoff errors (only for kdyn=3).