Skip to content

Fix MATX_ASSERT_COMPATIBLE_OP_SIZES macro checks of first N-1 dims#1135

Merged
tbensonatl merged 3 commits intomainfrom
bugfix/fix-several-issues-identified-by-opus-review
Mar 10, 2026
Merged

Fix MATX_ASSERT_COMPATIBLE_OP_SIZES macro checks of first N-1 dims#1135
tbensonatl merged 3 commits intomainfrom
bugfix/fix-several-issues-identified-by-opus-review

Conversation

@tbensonatl
Copy link
Copy Markdown
Collaborator

The MATX_ASSERT_COMPATIBLE_OP_SIZES macro checks for compatible sizes between the dimensions of two operators. It previously replace the compatible flag when computing it for each dimension and thus would only throw an error when operators differed in the last dimension.

This PR fixes the check such that compatible is only true if true for all dimensions. It also adds new unit tests that would fail without the fix.

The MATX_ASSERT_COMPATIBLE_OP_SIZES macro checks for compatible sizes
between the dimensions of two operators. It previously replace the
compatible flag when computing it for each dimension and thus would
only throw an error when operators differed in the last dimension.

This PR fixes the check such that compatible is only true if true
for all dimensions. It also adds new unit tests that would fail
without the fix.

Signed-off-by: Thomas Benson <tbenson@nvidia.com>
@tbensonatl tbensonatl self-assigned this Mar 8, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Mar 8, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 8, 2026

Greptile Summary

This PR fixes four distinct but related bugs around dimension/size checking in MatX operators, and adds regression tests to cover each case.

  • include/matx/core/error.h — Core fix: compatible =compatible &= in MATX_ASSERT_COMPATIBLE_OP_SIZES. Without this change the loop overwrote the flag on every iteration, so only the last dimension was ever validated.
  • include/matx/operators/cov.hCovOp constructor now correctly assigns the last two output dimensions as {M, M} (where M = a.Size(Rank()-1)) instead of naively copying all input dimensions; previously the second-to-last output dimension was set to the number of observations rather than the number of variables.
  • include/matx/transforms/cov.h — Batch-dimension equality check in matxCovHandle_t now starts from index 0 instead of 2, ensuring the first two batch dimensions are also validated.
  • include/matx/operators/conv.hConv2DOp constructor now computes per-tensor dimension indices (a_idx, b_idx) when OpA::Rank() != OpB::Rank(), preventing incorrect or out-of-bounds Size() calls on the lower-rank tensor.
  • New unit tests in complex_cast_exceptions_test.cu and Cov.cu exercise all fixed code paths and would have caught the original bugs.

Confidence Score: 5/5

  • All changes are targeted, well-reasoned bug fixes with comprehensive new tests; safe to merge.
  • Each fix is minimal and directly addresses a documented bug. The one-character change in error.h (=&=) is the simplest possible correct fix. The cov.h and transforms/cov.h changes are self-consistent and validated by new typed tests. The conv.h index-offset fix is logically sound for all rank combinations ≥ 2, which is the only valid input for a 2D convolution operator. New regression tests cover the previously failing scenarios for every touched code path.
  • No files require special attention.

Last reviewed commit: ff6cd3d

@tbensonatl tbensonatl requested a review from cliffburdick March 9, 2026 00:48
@tbensonatl
Copy link
Copy Markdown
Collaborator Author

/build

Signed-off-by: Thomas Benson <tbenson@nvidia.com>
@tbensonatl
Copy link
Copy Markdown
Collaborator Author

/build

Signed-off-by: Thomas Benson <tbenson@nvidia.com>
@tbensonatl
Copy link
Copy Markdown
Collaborator Author

/build

@tbensonatl tbensonatl merged commit 22d8944 into main Mar 10, 2026
1 check passed
@tbensonatl tbensonatl deleted the bugfix/fix-several-issues-identified-by-opus-review branch March 10, 2026 18:38
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