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

Pass new seaice_melt field from SIS2 to MOM6 #710

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

Conversation

hdrake
Copy link

@hdrake hdrake commented Aug 15, 2024

@hdrake hdrake changed the title Attempt to pass new seaice_melt field through the GFDL MOM6-SIS2 coupler Attempt to pass new seaice_melt field through the GFDL MOM6-SIS2 coupler Aug 15, 2024
@hdrake hdrake marked this pull request as draft August 15, 2024 17:27
@hdrake hdrake changed the title Attempt to pass new seaice_melt field through the GFDL MOM6-SIS2 coupler Attempt to new seaice_melt field from SIS2 to MOM6 Aug 16, 2024
@hdrake hdrake marked this pull request as ready for review August 16, 2024 23:54
@hdrake hdrake changed the title Attempt to new seaice_melt field from SIS2 to MOM6 Pass new seaice_melt field from SIS2 to MOM6 Aug 16, 2024
@@ -446,6 +447,12 @@ subroutine convert_IOB_to_fluxes(IOB, fluxes, index_bounds, Time, valid_time, G,
call check_mask_val_consistency(IOB%fprec(i-i0,j-j0), G%mask2dT(i,j), i, j, 'fprec', G)
endif

if (associated(IOB%seaice_melt)) then

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marshallward Is this sufficient to make sure that fluxes%seaice_melt is not filled with nonsense if IOB%seaice_melt is not allocated or used by the coupler?

Copy link
Member

@marshallward marshallward Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say prevent, it only ensures that the potential nonsense from IOB%seaice_melt is transferred to fluxes%seaice_melt.

The only solution I can see is for net_FW to have some awareness of the state of IOB%seaice_melt. If the coupler fills IOB%seaice_melt, then it can be used to compute new_FW (via fluxes%seaice_melt). Otherwise, it needs to be filled some other way (if at all).

Zero-initialization might get you out of this, although generally we try to avoid that solution where possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing there is no way to check if IOB%seaice_melt has been allocated with actual values?

What if we add use_seaice_melt to IOB as a logical, default it to false and then set it to true in the coupler where IOB%seaice_melt was allocated? Then use that here to see if seaice_melt should be used or not in this routine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some in preson discussion @marshallward confirmed the if associated should be enough to make sure the pointer was allocated in the coupler. There should be no need to change anything to get this working with older versions of the coupler.

@theresa-morrison
Copy link

Were you able to test the changes to the non-FMS caps with the associated couplers?

@theresa-morrison
Copy link

Is the expected behavior here that lprec_new + seaice_melt = lprec_old? And if so, do you find that to be the case?
lprec_new being the the liquid precititation into the ocean after these changes and lprec_old being the liquid precititation before these changes.

@hdrake
Copy link
Author

hdrake commented Aug 19, 2024

Is the expected behavior here that lprec_new + seaice_melt = lprec_old? And if so, do you find that to be the case?
lprec_new being the the liquid precititation into the ocean after these changes and lprec_old being the liquid precititation before these changes.

Yes, that is the expected behavior. I have not done a careful side by side comparison to verify they are the same, but visually they look like it.

What kind of workflow do you use to directly compare two configurations? Checkout the different commits and manually run different configurations and compare the outputs?

@hdrake
Copy link
Author

hdrake commented Aug 19, 2024

Were you able to test the changes to the non-FMS caps with the associated couplers?

No, I've only tested with the Baltic_OM4_05 configuration that I believe uses the FMS cap.

@theresa-morrison
Copy link

What kind of workflow do you use to directly compare two configurations? Checkout the different commits and manually run different configurations and compare the outputs?

Yes. Checkout and compile the dev/gfdl code, run and save lprec. Then recompile with all changes are rerun the experiment saving lprec and seaice_melt. I think it would be sufficient to test the final versions of MOM6, SIS2, and the coupler and not every commmit along the way (assuming of course that it works).

@hdrake
Copy link
Author

hdrake commented Aug 19, 2024

Were you able to test the changes to the non-FMS caps with the associated couplers?

@theresa-morrison, are these particular experiments I should test with? I guess all of the ones named SIS2_* in the MOM6-examples/ice_ocean_SIS2?

@hdrake
Copy link
Author

hdrake commented Aug 20, 2024

Is the expected behavior here that lprec_new + seaice_melt = lprec_old? And if so, do you find that to be the case?
lprec_new being the the liquid precititation into the ocean after these changes and lprec_old being the liquid precititation before these changes.

Below is a comparison between the current MOM6-examples/dev/gfdl and my three PRs.

Screenshot 2024-08-19 at 5 13 46 PM

I do not have any idea why the two are different. I don't see how I could have gone wrong in separating the diagnostics in the source code...

@hdrake
Copy link
Author

hdrake commented Aug 20, 2024

Is it possible that the answers with default parameter values have changed this much?

The boundary mass flux diagnostics seem internally consistent within each of the simulations:
Screenshot 2024-08-19 at 6 05 22 PM

(These residuals are about 1e-5 each of the individual terms in the budget, so indistinguishable from zero at single precision.)

@theresa-morrison
Copy link

There should be no parameter changes between the two experiments!
If you are using the version of MOM6 in MOM6 examples for the "old" experiment, you should instead use the version of dev/gfdl that this was branched from to make sure you have all the same defaults.

@hdrake
Copy link
Author

hdrake commented Aug 20, 2024

If you are using the version of MOM6 in MOM6 examples for the "old" experiment, you should instead use the version of dev/gfdl that this was branched from to make sure you have all the same defaults.

Okay, yeah that makes sense. I'll figure out how to do that and regenerate these plots.

@marshallward
Copy link
Member

Some additional comments

  • The changes to STALE_mct_cap and nuopc_cap should be removed. These caps are not meant to be used with the FMS coupler, and we don't maintain them. They already have their methods for handling ice-ocean boundaries.

    Additionally, STALE_mct_cap is an archived copy of an unmaintained MCT cap, so it should never be changed.

  • Are modifications to MESO permitted? I am unsure if it's meant to be an archive of the original experiments, or if we should extend its support? (I suppose it should give identical answers either way? Maybe this is OK. I will confirm.)

    Similar comments for BFB and dumbbell, although they are probably OK.

@hdrake
Copy link
Author

hdrake commented Aug 20, 2024

There should be no parameter changes between the two experiments!
If you are using the version of MOM6 in MOM6 examples for the "old" experiment, you should instead use the version of dev/gfdl that this was branched from to make sure you have all the same defaults.

Okay, I did a clean comparison where the only changes were those in my three branches, and I confirmed there were no differences between the MOM_parameter_doc.all and SIS_parameter_doc.all files. However, the differences in the outputs shown above persisted. So it seems I must be doing something wrong in one of these PRs.

@hdrake hdrake marked this pull request as draft August 20, 2024 17:00
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.91%. Comparing base (9b45087) to head (13b9960).

Additional details and impacted files
@@             Coverage Diff              @@
##           dev/gfdl     #710      +/-   ##
============================================
- Coverage     37.03%   35.91%   -1.13%     
============================================
  Files           273      270       -3     
  Lines         82538    81496    -1042     
  Branches      15441    15427      -14     
============================================
- Hits          30568    29269    -1299     
- Misses        46253    46458     +205     
- Partials       5717     5769      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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