Fix rotation in set_coupler_type_data#813
Conversation
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
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. |
Hallberg-NOAA
force-pushed
the
fix_ct_rotation
branch
from
d3db7b5 to
49eb64c
Compare
Codecov ReportAttention: Patch coverage is
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. |
|
I added a commit which updates (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
force-pushed
the
fix_ct_rotation
branch
from
2299b35 to
b7bc87f
Compare
|
This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26161. |
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
These changes seem sensible to me, with less of a chance of encountering compiler-specific problems.
`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
force-pushed
the
fix_ct_rotation
branch
from
b7bc87f to
32922a4
Compare
2 participants
rotate_arrayinset_coupler_type_datawas trying to rotate an array to another of different size whenidimandjdimare 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
idimandjdimare passed toCT_set_data, which (hopefully) sorts out the indexing.