Skip to content

Fix bug in Heat.indexing.nonzero#2138

Merged
brownbaerchen merged 3 commits intomainfrom
bugs/2136-_Bug_Bug_in_heat_indexing_nonzero
Mar 10, 2026
Merged

Fix bug in Heat.indexing.nonzero#2138
brownbaerchen merged 3 commits intomainfrom
bugs/2136-_Bug_Bug_in_heat_indexing_nonzero

Conversation

@brownbaerchen
Copy link
Collaborator

@brownbaerchen brownbaerchen commented Feb 20, 2026 •
edited
Loading

Due Diligence

  • General:
  • 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

This fixes the bugs documented in #2136 and #2135. It makes the function heat.indexing.nonzero compatible with the sanity checks proposed in #2137.

Issue/s resolved: #2136, #2135

Changes proposed:

  • Refactor and bugfix in heat.indexing.nonzero

Type of change

  • bug fix

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

no

@brownbaerchen brownbaerchen requested a review from mtar February 20, 2026 09:18
@github-project-automation github-project-automation bot moved this to Todo in Roadmap Feb 20, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

Thank you for the PR!

@codecov
Copy link

codecov bot commented Feb 20, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.63%. Comparing base (09a2a6f) to head (c1e8c2b).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2138      +/-   ##
==========================================
- Coverage   91.63%   91.63%   -0.01%     
==========================================
  Files          89       89              
  Lines       14016    14012       -4     
==========================================
- Hits        12844    12840       -4     
  Misses       1172     1172
Flag Coverage Δ
unit 91.63% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mtar
mtar requested changes Mar 2, 2026
View reviewed changes
Copy link
Collaborator

@mtar mtar left a comment

Choose a reason for hiding this comment

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

Thanks, the PR looks fine. The added edge case doesn't work properly. Something's wrong with the indexing.

heat/core/tests/test_indexing.py Outdated
Comment on lines +27 to +32
for split in [None, 1]:
a = ht.zeros((4, 3), dtype=ht.bool, split=split)
a[1, 2] = True
nz = ht.indexing.nonzero(a)
self.assertEqual(nz.gshape, (1, 2))
self.assertTrue(ht.allclose(a[nz], a[a]))
Copy link
Collaborator

@mtar mtar Mar 2, 2026

Choose a reason for hiding this comment

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

The edge case for split=1 doesn't work. All local tensors have no data after indexing.

print(a.comm.rank, a[nz].larray)
print(a.comm.rank, a[a].larray)

0 tensor([], dtype=torch.bool)
1 tensor([], dtype=torch.bool)

0 tensor([], dtype=torch.bool)
1 tensor([], dtype=torch.bool)

Copy link
Collaborator Author

@brownbaerchen brownbaerchen Mar 3, 2026

Choose a reason for hiding this comment

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

That's true, good catch!

This PR is kind of working on two issues. The first is in heat.indexing.nonzero and the second is in the advanced indexing. Turns out this fixes the advanced indexing bug only when the array is non-distributed.
However, the primary function of this PR, which is fixing the bug in nonzero is still valid. I modified the test to index only non-distributed data. Note that this test fails on the current main.

I suggest we merge this anyways and hope that #938 will fix the indexing.

@github-project-automation github-project-automation bot moved this from Todo to In Progress in Roadmap Mar 2, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2026

Thank you for the PR!

@ClaudiaComito ClaudiaComito added this to the 1.8.0 milestone Mar 3, 2026
@github-project-automation github-project-automation bot moved this from In Progress to Merge queue in Roadmap Mar 9, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

Thank you for the PR!

@brownbaerchen brownbaerchen force-pushed the bugs/2136-_Bug_Bug_in_heat_indexing_nonzero branch from d92a30d to c1e8c2b Compare March 10, 2026 13:44
@brownbaerchen brownbaerchen merged commit 4a8bf3d into main Mar 10, 2026
20 checks passed
@github-project-automation github-project-automation bot moved this from Merge queue to Done in Roadmap Mar 10, 2026
@brownbaerchen brownbaerchen deleted the bugs/2136-_Bug_Bug_in_heat_indexing_nonzero branch March 10, 2026 14:21
@github-actions github-actions bot mentioned this pull request Mar 10, 2026
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2026

Successfully created backport PR for stable:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mtar mtar mtar approved these changes

Assignees

No one assigned

Labels

backport stable bug Something isn't working core indexing

Projects

Roadmap
Status: Done

Milestone

1.8.0

Development

Successfully merging this pull request may close these issues.

[Bug]: Bug in heat.indexing.nonzero

3 participants

@brownbaerchen @mtar @ClaudiaComito