Skip to content

Conversation

@msimberg
Copy link
Contributor

Makes sure the correct datatype is selected for the identity values when a rank has no local data. Also chooses inf/-inf for floats and (finite) min/max values for integral types.

Also adds one more test input with integral values.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes identity values used in global reductions when a rank has no local data. The changes ensure proper datatype selection by using infinity for floating-point types and min/max integer limits for integral types. Additionally, a new test case with integer values is added to verify the fix.

Changes:

  • Added new static helper methods to compute appropriate identity values for min, max, and sum operations based on the data type
  • Updated global reduction methods to use these type-aware identity values instead of hardcoded values
  • Added integer test case [-10, 20, 4] and updated type hints to support both float and integer inputs

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py Adds three static identity methods and updates min/max/sum/mean to use dtype-specific identity values
model/common/tests/common/decomposition/mpi_tests/test_mpi_decomposition.py Adds integer test input and updates type hints from list[float] to list[data_alloc.ScalarT]

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…osition.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@msimberg msimberg requested a review from nfarabullini January 22, 2026 13:52
@msimberg msimberg marked this pull request as ready for review January 22, 2026 13:52
@msimberg
Copy link
Contributor Author

@nfarabullini would you mind being manual CI for this PR, and checking that the tests pass for you locally as well? I've tested this locally where it passes (but the tests passed previously as well). I've also added the changes to #692 where the tests used to fail but now pass.

@nfarabullini
Copy link
Contributor

cscs-ci run default

@msimberg
Copy link
Contributor Author

cscs-ci run default

@msimberg msimberg force-pushed the fix-global-reduction-identity branch from d57eea5 to 5b6c150 Compare January 22, 2026 14:20
@github-actions
Copy link

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark-bencher

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:

  • cscs-ci run extra

For more detailed information please look at CI in the EXCLAIM universe.

@msimberg
Copy link
Contributor Author

cscs-ci run default

@msimberg msimberg merged commit 912c2bf into C2SM:main Jan 22, 2026
69 of 383 checks passed
@msimberg msimberg deleted the fix-global-reduction-identity branch January 22, 2026 17:01
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