Fix nonzero for non-standard input layouts - #5046
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5046 +/- ##
===========================================
+ Coverage 92.78% 92.90% +0.12%
===========================================
Files 596 603 +7
Lines 32011 32524 +513
===========================================
+ Hits 29700 30214 +514
+ Misses 2311 2310 -1
🚀 New features to boost your workflow:
|
pfultz2
reviewed
Jul 8, 2026
Regressions detected 🔴 |
|
bdevorem
approved these changes
Jul 15, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the reference implementation of the nonzero operator to correctly handle non-standard input layouts (e.g., transposed and broadcasted tensors), aligning compute-time element access with logical indexing.
Changes:
- Removed the standard-layout constraint from
nonzeroshape validation and updated compute-time access to use logical indices fromshape_for_each. - Added new reference + verify tests covering transposed inputs and reference coverage for broadcasted inputs.
- Added a changelog entry under “Resolved issues”.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/include/migraphx/op/nonzero.hpp |
Removes standard-layout requirement and updates indexing logic to support non-standard layouts. |
test/ref/nonzero.cpp |
Adds reference tests for transposed and broadcasted inputs. |
test/verify/test_nonzero.cpp |
Adds verify coverage for transposed inputs. |
CHANGELOG.md |
Documents the fix under resolved issues. |
Comment on lines
56
to
60
| @@ -56,8 +58,8 @@ struct nonzero | |||
| std::vector<std::vector<std::size_t>> vec_idx; | |||
| auto s = args.front().get_shape(); | |||
| args.front().visit([&](auto v) { | |||
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
TedThemistokleous
self-requested a review
July 16, 2026 14:15
TedThemistokleous
requested changes
Jul 16, 2026
TedThemistokleous
left a comment
TedThemistokleous
left a comment
Collaborator
There was a problem hiding this comment.
Just need clarification on your test data. I think you have extra values that aren't checked / dont make sense.
Possible to also clean up the output assignment with a transform using migraphx ranges for front().size instead of keeping the raw loop.
CharlieL7
approved these changes
Jul 21, 2026
CharlieL7
left a comment
CharlieL7
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, tests work out correctly vs. what I did by hand.
Collaborator
|
Need to fix Tidy CI check |
TedThemistokleous
approved these changes
Jul 21, 2026
TedThemistokleous
left a comment
TedThemistokleous
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the clarification and contribution. Looks good to me too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Fix the reference
nonzerooperator so it accepts and correctly reads non-standard input layouts such as transposed and broadcasted tensors.Technical Details
Removed the standard-layout requirement from
nonzeroshape checking and changed compute-time element access to use the logical index fromshape_for_each. Added reference and verify tests for transposed inputs, plus reference coverage for broadcasted inputs.Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable