-
Notifications
You must be signed in to change notification settings - Fork 65
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
Refactor Axis Information Handling for Improved Performance in ocean_sponge #653
Refactor Axis Information Handling for Improved Performance in ocean_sponge #653
Conversation
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.
Although these changes appear to fix the specific problem with FMS2 being incredibly slow when reading the sponge data, the way that they do so are problematic. Specifically, by saving this information via the save
attribute, these changes would preclude the use of the horizontal regridding with multiple files with different axis information. Moreover, the save
attribute is essentially the same as adding module data, which is not permitted in MOM6 as described at https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide#global--module-data.
One acceptable way to accomplish what is being attempted here this would be to add a type as an optional argument to store this information between calls, with the actual storage of this type occurring at a higher level (probably in the control structure for the sponges). This would allow this code to be reused with multiple files.
e63a4c5
to
5057aee
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #653 +/- ##
============================================
+ Coverage 36.92% 37.00% +0.07%
============================================
Files 271 271
Lines 81545 81214 -331
Branches 15244 15149 -95
============================================
- Hits 30112 30050 -62
+ Misses 45791 45534 -257
+ Partials 5642 5630 -12 ☔ View full report in Codecov by Sentry. |
Thank you for the detailed feedback. I have updated this PR to address the issues you mentioned. Specifically, I have removed the Please feel free to review the changes and let me know if any further adjustments are needed. |
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.
This is now an elegant solution to the performance problems that had been noted, and it all looks good to me.
@yichengt900 For reasons I still don't understand, we're not able to rebase or squash PRs from organizations. Somehow or another, we need to allow NOAA-GFDL (or NOAA-GFDL/MOM6) to modify PRs from NOAA-GFDL-Regional-Ocean's MOM6. I tried to work this out with @kshedstrom but did not get very far. There are three options here:
Typically we have done option 3, but this might be a good opportunity to finally get option 1 working. |
fix style errors Modify axes_data to make it allocatable fix style
4c49ec9
to
f7aef94
Compare
Hi @marshallward, hmm I haven't figured out how to make option 1 work yet and will need to spend more time on it. In the meantime, I have squashed the commits, so please feel free to merge if you see fit. |
Of course I forgot option 4: Do the rebase on your end 🤣 Really appreciated, thanks. We can look more into it when it happens next time. |
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/23703 ✔️ |
This PR addresses issue #644 by introducing a logic change when calling
get_external_field_info
inMOM_interp_infra.F90
. Since the axis, dimension, and missing value information for nudging are time-independent, there is no need to read this information from the NetCDF file at every time step when usingFMS2
. With this fix, the runtime of nudging experiments withFMS1
andFMS2
are now comparable:FMS2 with new fix one year run:
FMS1 with new fix one year run:
Keep in mind, this is just a simple fix to allow users to run MOM6 with both the sponge option and FMS2 simultaneously. It would probably be better to refactor
FMS2/MOM_interp_infra.F90
in the future to fundamentally solve the issue.