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

Fix rotation in set_coupler_type_data #813

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

marshallward
Copy link
Member

rotate_array in set_coupler_type_data was trying to rotate an array to another of different size when idim and jdim are present. Some compilers seemed to let this through, but it raised a double-deallocation error in GCC.

I'm guessing it's because the rotation was implicitly allocating to a new array which was automatically deallocated. But I did not confirm this.

This was modified to rotate onto a new array of the same size. The idim and jdim are passed to CT_set_data, which (hopefully) sorts out the indexing.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

While I do not fully understand why the original code was failing, I agree that the revised code will work as intended.

@marshallward
Copy link
Member Author

While I do not fully understand why the original code was failing, I agree that the revised code will work as intended.

The arrays were different sizes. The rotation function doesn't support that.

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 21.94%. Comparing base (576fb41) to head (2299b35).

Files with missing lines Patch % Lines
src/framework/MOM_coupler_types.F90 0.00% 11 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (576fb41) and HEAD (2299b35). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (576fb41) HEAD (2299b35)
2 1
Additional details and impacted files
@@              Coverage Diff              @@
##           dev/gfdl     #813       +/-   ##
=============================================
- Coverage     38.15%   21.94%   -16.21%     
=============================================
  Files           296      136      -160     
  Lines         87248    32813    -54435     
  Branches      16283     5836    -10447     
=============================================
- Hits          33289     7202    -26087     
+ Misses        47975    25054    -22921     
+ Partials       5984      557     -5427     

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

@marshallward
Copy link
Member Author

I added a commit which updates extract_coupler_type_data to use the same method as in set_coupler_type_data. Hopefully identical, but it avoids two implicit copies in the allocate_rotated_array and rotate_array calls.

(This function would not have seen this error, since both functions were modified and arrays shapes were identical.)

Hope I didn't interfere with testing. 🤐

@Hallberg-NOAA
Copy link
Member

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26161.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

These changes seem sensible to me, with less of a chance of encountering compiler-specific problems.

marshallward and others added 2 commits January 23, 2025 05:03
`rotate_array` in `set_coupler_type_data` was trying to rotate an array
to another of different size when `idim` and `jdim` are present.  Some
compilers seemed to let this through, but it raised a
double-deallocation error in GCC.

I'm guessing it's because the rotation was implicitly allocating to a
new array which was automatically deallocated.  But I did not confirm
this.

This was modified to rotate onto a new array of the same size.  The
`idim` and `jdim` are passed to `CT_set_data`, which (hopefully) sorts
out the indexing.
Following on the previous commit, the implicit copies in
`extract_coupler_type_data`'s `allocate_rotated_array` and
`rotate_array` are replaced with the full-sized arrays, with halos
managed by `idim` and `jdim` arguments to `CT_extract_data`.

I tested this in a rotated `Baltic` test and saw no answer changes.  The
`ocean.stats` and CFC restart files agree, but there are still known
rotation reordering and negative-zero errors in `MOM.res.nc` output.
@Hallberg-NOAA Hallberg-NOAA merged commit 320aac2 into NOAA-GFDL:dev/gfdl Jan 23, 2025
10 checks passed
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.

2 participants