-
Notifications
You must be signed in to change notification settings - Fork 60
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
base: dev/gfdl
Are you sure you want to change the base?
Conversation
seaice_melt
field through the GFDL MOM6-SIS2 coupler
Some other minor bug fixes
seaice_melt
field through the GFDL MOM6-SIS2 couplerseaice_melt
field from SIS2 to MOM6
seaice_melt
field from SIS2 to MOM6seaice_melt
field from SIS2 to MOM6
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Were you able to test the changes to the non-FMS caps with the associated couplers? |
Is the expected behavior here that |
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? |
No, I've only tested with the |
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). |
@theresa-morrison, are these particular experiments I should test with? I guess all of the ones named |
Below is a comparison between the current 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... |
There should be no parameter changes between the two experiments! |
Okay, yeah that makes sense. I'll figure out how to do that and regenerate these plots. |
Some additional comments
|
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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
See companion PRs NOAA-GFDL/SIS2#214 and NOAA-GFDL/FMScoupler#145