Skip to content

Fix bug leading to mean and var being NaN if calculated distributed - #2495

Closed
maliesen wants to merge 9 commits into
helmholtz-analytics:mainfrom
maliesen:fix/merge-moments-empty-ranks
Closed

Fix bug leading to mean and var being NaN if calculated distributed#2495
maliesen wants to merge 9 commits into
helmholtz-analytics:mainfrom
maliesen:fix/merge-moments-empty-ranks

Conversation

@maliesen

Copy link
Copy Markdown
Contributor

Due Diligence

  • General:
    • [ X] title of the PR is suitable to appear in the Release Notes
  • Implementation:
    • unit tests: all split configurations tested
    • unit tests: multiple dtypes tested
    • NEW unit tests: MPS tested (1 MPI process, 1 GPU)
    • benchmarks: created for new functionality
    • benchmarks: performance improved or maintained
    • documentation updated where needed

Description

If the data is distributed so that the first two leading ranks are empty a divison by zero in merge_moments leads to mean and variance being calculated as nan. This can be circumvented by early returning what has been merged so far if there is no new actual data coming in.

Issue/s resolved: #

Changes proposed:

Type of change

Bug fix (non-breaking change which fixes an issue)

Memory requirements

Performance

Does this change modify the behaviour of other functions? If so, which?

yes / no

@brownbaerchen brownbaerchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for spotting this! I have some minor comments but the overall fix is good.

Comment thread tests/core/test_statistics.py Outdated
Comment thread heat/core/statistics.py Outdated
Comment thread heat/core/statistics.py Outdated
Comment thread tests/core/test_statistics.py Outdated
Comment thread tests/core/test_statistics.py Outdated
@github-project-automation github-project-automation Bot moved this from Todo to In Progress in Roadmap Aug 27, 2026
maliesen and others added 3 commits August 28, 2026 19:46
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
@maliesen

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

@maliesen
maliesen requested a review from brownbaerchen August 29, 2026 07:54
@maliesen
maliesen marked this pull request as draft August 29, 2026 10:49
@maliesen

Copy link
Copy Markdown
Contributor Author

I put the pr in draft made as I realized the fix as is would introduce the following regression:

    @unittest.skipUnless(ht.communication.MPI_WORLD.size == 2, "Test for two tasks")
    def test_corrected_var_single_element(self):
        comm = self.comm
        ltensor_empty = torch.tensor([], dtype=torch.float32)
        ltensor_data = torch.tensor([0.], dtype=torch.float32)

        ldata_on_first_rank = ltensor_data if comm.rank == 0 else ltensor_empty
        data_on_first_rank = ht.DNDarray(ldata_on_first_rank, gshape=(2,), dtype=ht.float32, split=0, device=ht.devices.cpu, comm=comm, balanced=False)

        self.assertTrue(ht.isnan(ht.var(data_on_first_rank, ddof=1)))

This is because if the data (single element) is on the first rank, torch will give nan in https://github.com/helmholtz-analytics/heat/blob/main/heat/core/statistics.py#L2226 which will then be normalized to 0 in https://github.com/helmholtz-analytics/heat/blob/main/heat/core/statistics.py#L2228-L2229 so the merge_moments function is responsible for turning this in nan at https://github.com/helmholtz-analytics/heat/blob/main/heat/core/statistics.py#L1132 which would not happen anymore by the early return in this fix.

The distributed calculation of the corrected variance has a nan bug already before this fix (in the __moment_w_axis code branch) and I planned on fixing this as a separate pr but due to the regression I think it is better to do it at once. Therefore I merged the fix of this pr into #2507 and if that pr is successful this pr may be closed.

@brownbaerchen

Copy link
Copy Markdown
Collaborator

Closing as superseded by #2507.

@github-project-automation github-project-automation Bot moved this from In Progress to Done in Roadmap Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants