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

Separating sea ice melt/formation from liquid precipitation field #214

Open
wants to merge 4 commits into
base: dev/gfdl
Choose a base branch
from

Conversation

hdrake
Copy link

@hdrake hdrake commented Aug 15, 2024

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_melt has 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)

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 hdrake marked this pull request as draft August 15, 2024 00:01
@hdrake hdrake changed the title Attempt at separating sea ice melt/formation from liquid precipitation Separating sea ice melt/formation from liquid precipitation field Aug 15, 2024
@hdrake
Copy link
Author

hdrake commented Aug 15, 2024

This is the line that is currently giving me problems when running:

https://github.com/NOAA-GFDL/MOM6/blob/b3e65c63533bf17ae3dd70243f8827fa277ff386/config_src/drivers/FMS_cap/MOM_surface_forcing_gfdl.F90#L1799

If I remove it, the model runs (I am using the Baltic_OM4_05 configuration for testing, with a diag_table that includes fsitherm from the ocean_model) but then the fsitherm diagnostic stays full of zero and is not being currently overwritten by the coupling with SIS2.

@hdrake
Copy link
Author

hdrake commented Aug 16, 2024

@theresa-morrison pointed out that that I forgot to allocate the new seaice_melt field in the coupler!

After doing so in this PR, my Baltic sea test now works!

Screenshot 2024-08-16 at 4 58 55 PM

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!
@theresa-morrison
Copy link

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?

@hdrake
Copy link
Author

hdrake commented Dec 13, 2024

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.

@Hallberg-NOAA
Copy link
Member

Hallberg-NOAA commented Dec 13, 2024

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!

@hdrake
Copy link
Author

hdrake commented Dec 17, 2024

Thanks for clarifying @Hallberg-NOAA. I do think answers will not change in the sense that the sum of the two existing diagnostics, prlq + fsitherm, will be bitwise identical. But for prlq and fsitherm individually the answers will definitely change.

Is this fine or should I add a runtime BUG flag? I assume I can follow the implementation for something like this:

SIS2/src/ice_model.F90

Lines 214 to 223 in 2c49005

if (Ice%sCS%berg_windstress_bug) then
! This code is only required to reproduce an old bug.
i_off = LBOUND(Ice%flux_t,1) - sG%isc
j_off = LBOUND(Ice%flux_t,2) - sG%jsc
!$OMP parallel do default(none) shared(Ice,sG,US,i_off,j_off) private(i2,j2)
do j=sG%jsc,sG%jec ; do i=sG%isc,sG%iec
i2 = i+i_off ; j2 = j+j_off
Ice%sCS%IOF%flux_u_ocn(i,j) = US%kg_m2s_to_RZ_T*US%m_s_to_L_T*Ice%flux_u(i2,j2)
Ice%sCS%IOF%flux_v_ocn(i,j) = US%kg_m2s_to_RZ_T*US%m_s_to_L_T*Ice%flux_v(i2,j2)
enddo ; enddo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants