+Add symmetric_sum functions#1069
Conversation
adcroft
left a comment
adcroft
left a comment
There was a problem hiding this comment.
A few issues found in the new code — see inline comments. Also, the symmetric_sum_unit_tests function is not reachable via make run.unit (the .testing unit test suite). That path auto-discovers standalone programs in config_src/drivers/unit_tests/. A new file config_src/drivers/unit_tests/test_MOM_array_transform.F90 is needed, following the pattern of e.g. test_MOM_string_functions.F90:
! This file is part of MOM6, the Modular Ocean Model version 6.
! See the LICENSE file for licensing information.
! SPDX-License-Identifier: Apache-2.0
program test_MOM_array_transform
use MOM_array_transform, only : symmetric_sum_unit_tests
use MOM_error_handler, only : set_skip_mpi
character(len=120) :: fail_message
call set_skip_mpi(.true.) ! This unit test is not expecting MPI to be used
if ( symmetric_sum_unit_tests(fail_message) ) then
write(*,*) "FAILED: "//trim(fail_message)
stop 1
endif
end program test_MOM_array_transformNo Makefile changes are needed — the target auto-discovers all .F90 files in that directory.
| integer :: i, szi, szi_2 | ||
|
|
||
| szi = size(field, 1) | ||
| szi_2 = szi / 2 ! Note that for an add number sz_2 is rounded down. |
There was a problem hiding this comment.
Typo: "add number" should be "odd number".
There was a problem hiding this comment.
Good catch. It is only when this was pointed out that my brain was not automatically filling in "odd" for "add"!
| sum = symmetric_sum_1d(field(:,1)) | ||
| else | ||
| ! This is the general case. | ||
| ! Note that for an add number szi_2 is rounded down. |
There was a problem hiding this comment.
Typo: "add number" should be "odd number".
| real :: sum !< The rotation dependent sum of the entries in field [A ~> a] | ||
|
|
||
| ! Local variables | ||
| real :: quad_sum(2,2) ! The sums in each of the quadrants [A ~> a] |
There was a problem hiding this comment.
quad_sum is declared here but never used in naive_sum_2d. Most compilers will warn about this unused variable.
There was a problem hiding this comment.
Thank you for noting this; it has now been removed.
| call rotate_array_real_2d(array, 2, ar_180) | ||
| call rotate_array_real_2d(array, 3, ar_270) | ||
|
|
||
| symmetric_sum_unit_tests = fail |
There was a problem hiding this comment.
This assignment is dead code — fail is always .false. at this point, before the test loop runs. The correct final assignment is on line 565. This line should be removed.
There was a problem hiding this comment.
Good point. This line has been removed.
adcroft
left a comment
adcroft
left a comment
There was a problem hiding this comment.
This is a well-crafted PR — the recursive "Union-Jack" summation pattern is elegant, the unit tests are thorough (mixing positive and negative inexact values to amplify floating-point order-of-arithmetic differences is a nice touch), and it is great to see the unit test paradigm being followed with symmetric_sum_unit_tests. The code is clean, well-commented, and the dimensional annotation is consistent throughout.
A few small issues to address before merging:
- Missing standalone test driver:
symmetric_sum_unit_testsis not reachable viamake run.unit— a newconfig_src/drivers/unit_tests/test_MOM_array_transform.F90is needed (see the earlier review comment for a ready-to-use template). - Dead code on line 538: the first
symmetric_sum_unit_tests = failassignment (before the test loop) should be removed — see inline comment. - Two typos ("add" → "odd") in comments — see inline comments on lines 384 and 420.
- Unused variable
quad_suminnaive_sum_2d— see inline comment on line 488.
Added the new interfaces `symmetric_sum()` and `symmetric_sum_unit_tests()` to the MOM_array_transform module, and added a call to `symmetric_sum_unit_tests()` to `unit_tests()`. The two functions wrapped by the `symmetric_sum()` interface are intended to be a general template for rotationally symmetric sums, but they demonstrably work as intended (as demonstrated by the passing unit tests) for any rectangular arrays. The new driver in test_MOM_array_transform.F90 tests these new arrays. Although `symmetric_sum()` is not used yet in the MOM6 code apart from the tests, it was developed to address problems with failures of rotational symmetry in the diagnostic downscaling routines, and will be used there soon. All answers are bitwise identical, but there are two new publicly visible interface.
Hallberg-NOAA
force-pushed
the
add_symmetric_sum
branch
from
f8a0812 to
11fa4d1
Compare
|
In addition to correcting the noted errors, I have also added the new test-driver in test_MOM_array_transform.F90. It differs somewhat from what was suggested above in that it uses calls to |
2 participants
Added the new interfaces
symmetric_sum()andsymmetric_sum_unit_tests()to the MOM_array_transform module, and added a call tosymmetric_sum_unit_tests()tounit_tests(). The two functions wrapped by thesymmetric_sum()interface are intended to be a general template for rotationally symmetric sums, but they demonstrably work as intended (as demonstrated by the passing unit tests) for any rectangular arrays. Althoughsymmetric_sum()is not used yet in the MOM6 code apart from the tests, it was developed to address problems with failures of rotational symmetry in the diagnostic downscaling routines, and will be used there soon. All answers are bitwise identical, but there are two new publicly visible interface.