Skip to content

Linspace fix for high ranks#1144

Open
cliffburdick wants to merge 2 commits intomainfrom
linspace_fix
Open

Linspace fix for high ranks#1144
cliffburdick wants to merge 2 commits intomainfrom
linspace_fix

Conversation

@cliffburdick
Copy link
Copy Markdown
Collaborator

@cliffburdick cliffburdick commented Apr 3, 2026

Fix linspace static_assert failing when NUM_RC > 2

The static_assert in LinspaceOp::operator() checked sizeof...(indices) == NUM_RC, but the operator is always rank 2 when NUM_RC > 1. With 3+ firsts/lasts entries, the operator is called with 2 indices but the assert expected 3+, causing a compile error.

The static_assert in LinspaceOp::operator() checked
sizeof...(indices) == NUM_RC, but the operator is always rank 2 when
NUM_RC > 1. With 3+ firsts/lasts entries, the operator is called with
2 indices but the assert expected 3+, causing a compile error.

Added unit tests to cover these cases
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Apr 3, 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.

@cliffburdick
Copy link
Copy Markdown
Collaborator Author

/build

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 3, 2026

Greptile Summary

This PR fixes a compile error in LinspaceOp::operator() where a static_assert compared the number of variadic indices against NUM_RC (the number of rows or columns), but the operator is always rank 2 (i.e., called with 2 indices) when NUM_RC > 1. The fix correctly replaces NUM_RC with Rank(), which returns 1 when NUM_RC == 1 and 2 otherwise.

Key changes:

  • include/matx/generators/linspace.h: Change static_assert(sizeof...(indices) == NUM_RC, ...) to static_assert(sizeof...(indices) == Rank(), ...), correctly matching the actual rank of the operator (always 1 or 2, never NUM_RC when NUM_RC > 2).
  • test/00_operators/GeneratorTests.cu: Adds three new test cases covering NUM_RC = 3 (axis=0), NUM_RC = 3 (axis=1), and NUM_RC = 4 (axis=0), verifying both Size() dimensions and per-element values.
  • docs_input/api/creation/operators/linspace.rst: Adds a documentation example referencing the new test-4 case.

The fix is minimal, correct, and Rank() is already defined as static inline constexpr so it is valid to use inside a static_assert.

Confidence Score: 5/5

This PR is safe to merge — it is a minimal, well-reasoned bug fix with no logic changes beyond the corrected static_assert.

The root cause is clearly identified and the fix (Rank() instead of NUM_RC) is the correct constant to use. Rank() is already a static constexpr method, so it is valid inside static_assert. No P0 or P1 issues were found. Three new test cases verify the fixed behavior end-to-end, and documentation was updated accordingly.

No files require special attention.

Important Files Changed

Filename Overview
include/matx/generators/linspace.h Single-line fix: static_assert now uses Rank() (always 1 or 2) instead of NUM_RC, correctly matching the number of indices the operator is called with.
test/00_operators/GeneratorTests.cu Adds three comprehensive test cases for NUM_RC=3 (axis=0 and axis=1) and NUM_RC=4 (axis=0), covering Size() and per-element value correctness.
docs_input/api/creation/operators/linspace.rst Adds a documentation literalinclude block linking the new test-4 example for high-rank linspace usage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["linspace(firsts, lasts, count, axis)"] --> B["LinspaceOp<T, NUM_RC> constructed"]
    B --> C{"NUM_RC == 1?"}
    C -- Yes --> D["Rank() = 1"]
    C -- No --> E["Rank() = 2 (regardless of NUM_RC)"]
    D --> F["operator()(idx0)\nstatic_assert: sizeof...(indices) == Rank()"]
    E --> G["operator()(idx0, idx1)\nstatic_assert: sizeof...(indices) == Rank()"]
    G --> H{"axis_ == 0?"}
    H -- Yes --> I["firsts_[idx1] + steps_[idx1] * idx0"]
    H -- No --> J["firsts_[idx0] + steps_[idx0] * idx1"]
    F --> K["firsts_[0] + steps_[0] * idx0"]
Loading

Reviews (2): Last reviewed commit: "Added examples" | Re-trigger Greptile

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.

1 participant