Separating sea ice melt/formation from liquid precipitation field #214
Conversation
For unjustified legacy reasons, these have been bundled together in the past. They are not only bundled together in SIS2 variables, but also in the ice-ocean fluxes passed to the coupler and hence to MOM6. This commit attempts to address this Issue: NOAA-GFDL#213 However, I currently am getting a SEGFAULT error stating that `iobt%seaice_melt` has not been allocated memory when I try to do a checksum on it.
hdrake
changed the title
|
This is the line that is currently giving me problems when running: If I remove it, the model runs (I am using the |
|
@theresa-morrison pointed out that that I forgot to allocate the new After doing so in this PR, my Baltic sea test now works! 
|
This fixes a bug introduced in my previous two commits, in which I was unintentionally adding IOF%seaice_melt rate on top of the previous timesteps result, without realizing that it never gets set back to zero!
|
Is there any reason we would want to be able to save the old method for calculating net_melt = melt+ lprec? Or is prlq (new) + fsitherm = prlq (old) sufficient for recreating old results if needed? |
I personally think this would be undesirable, as it just propagates something that was already mislabelled and could lead to further misinterpretation. |
|
We always retain the ability to use the old calculations if there is any chance at all that anyone is using them, no matter how ugly the code was. We can wrap in a runtime BUG flag or an ANSWER_DATE flag, but we do not ever change other people's answers without warning and explicit consent. Keeping the new method as the only option is only sufficient if it can be configured to be bitwise identical to the old one! |
|
Thanks for clarifying @Hallberg-NOAA. I do think answers will not change in the sense that the sum of the two existing diagnostics, Is this fine or should I add a runtime BUG flag? I assume I can follow the implementation for something like this: Lines 214 to 223 in 2c49005 |
| if (CS%id_net_melt>0) call post_data(CS%id_net_melt, net_melt, CS%diag) | ||
| if (CS%id_CMOR_melt>0) call post_data(CS%id_CMOR_melt, net_melt, CS%diag) | ||
| if (CS%id_net_melt>0) call post_data(CS%id_net_melt, IOF%seaice_melt, CS%diag) | ||
| if (CS%id_CMOR_melt>0) call post_data(CS%id_CMOR_melt, IOF%seaice_melt, CS%diag) |
There was a problem hiding this comment.
to reproduce old answers, I think this should be okay, since IOF%seaice_melt and net_melt are the same. Right?
|
|
||
| call disable_SIS_averaging(CS%diag) | ||
|
|
||
| ! Combine the liquid precipitation with the net melt of ice and snow for |
There was a problem hiding this comment.
I think a bug fix flag here will work to get the old diagnostics, but it would be ideal (in my opinion) if there was a way to save sea ice melt and sea ice melt + lprec from the same simulation.
src/SIS_sum_output.F90
Outdated
| do j=jsc,jec ; do i=isc,iec | ||
| CS%water_in_col(i,j) = CS%water_in_col(i,j) - dt * & | ||
| ( ((FIA%runoff(i,j) + FIA%calving(i,j)) + & | ||
| IOF%seaice_melt(i,j) + & |
There was a problem hiding this comment.
Please add parentheses to specify the order with which IOF%seaice_melt is added to the other fields so that this diagnostic does not capriciously change with the level of optimization.
My preference would be for IOF%seaice_melt to be added first to IOF%lprec_ocn_top so that the answers more closely resemble what was done previously, but other options could be justified as long as the order of arithmetic is specified unambiguously.
There was a problem hiding this comment.
Thank you @Hallberg-NOAA; I've started to make the necessary changes wherever fluxes%lprec shows up.
I am not sure what to do here:
https://github.com/NOAA-GFDL/MOM6/blob/37d1385b2979a453c5b2c24d91bbea7c74c6a82e/src/core/MOM_forcing_type.F90#L2275
Is there any way to separately increment these averages for fluxes%lprec and fluxes%seaice_melt while preserving the overall order of operations?
There was a problem hiding this comment.
Similarly, in any if associated(fluxes%lprec) blocks, do I need to separately treat the two cases of if associated(fluxes%seaice_melt)?
Following @Hallberg-NOAA's suggestion (https://github.com/NOAA-GFDL/SIS2/pull/214/files#r1938255222), I have added parentheses so that `IOF%seaice_melt` is added to `lprec_ocn_top` before it is added to other contributions, preserving the original order of operations.
Continues to address @Hallberg-NOAA's [concern](NOAA-GFDL/SIS2#214 (comment)) that these diagnostics could change with the level of compiler optimization by adding parentheses to preserve the order in which fluxes are added together. However, there are some locations where I did not see how this was possible, as in this comment: NOAA-GFDL/SIS2#214 (comment)
|
Henri, we are planning to merge MOM6 and SIS2 in the near future (hopefully by the end of the calendar year at the latest). Combining the two code bases would make merging both this PR and the related MOM6 PR much easier. Would you be willing to wait until the merge happens for these code changes to be accepted? |
|
Oh, that's great. Yes, it would be much easier to reimplement these changes within the same code base. I figured out how to separate these fields using the sea ice diagnostics offline in the meantime anyways. |
3 participants
For unjustified legacy reasons, these have been bundled together in the past. They are not only bundled together in SIS2 variables, but also in the ice-ocean fluxes passed to the coupler and hence to MOM6.
This commit attempts to address this Issue: #213
However, I currently am getting a SEGFAULT error stating that
iobt%seaice_melthas not been allocated memory when I try to do a checksum on it.(Edit: See compaion PRs NOAA-GFDL/MOM6#710 and NOAA-GFDL/FMScoupler#145)