Skip to content

Fix Shape operator specification: correct range bounds and document start > end behavior#7132

Merged
gramalingam merged 7 commits intomainfrom
copilot/fix-6862
Jul 16, 2025
Merged

Fix Shape operator specification: correct range bounds and document start > end behavior#7132
gramalingam merged 7 commits intomainfrom
copilot/fix-6862

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jul 16, 2025 •
edited
Loading

The Shape operator specification incorrectly stated that axes are clamped to the range [0, r-1], but since the end parameter is exclusive in slicing operations, the correct range should be [0, r]. Additionally, the spec was missing documentation for the start > end case.

Changes Made

Documentation Fixes

  • Corrected range specification: Changed from [0, r-1] to [0, r] in all Shape operator documentation
  • Added start > end behavior: Added "If start > end, the result will be an empty shape." to clarify edge case behavior

Affected Versions

Updated documentation for all Shape operator versions that support start and end attributes:

  • Shape_ver24_doc (current version)
  • Shape_ver23_doc
  • Shape_ver19_doc
  • Shape_ver15_doc

Note: Older versions (ver13, ver1) don't have start/end attributes so no changes were needed.

Test Coverage

  • Added test case test_shape("_start_greater_than_end", x, start=2, end=1) to validate the documented behavior
  • Verified that the reference implementation correctly returns an empty array when start > end

Implementation Verification

The underlying C++ implementation was already correct - it properly clamps both start and end to [0, rank] and handles the start > end case by returning an empty shape:

start = (start < 0) ? 0 : (start > rank) ? rank : start;
end = (end < 0) ? 0 : (end > rank) ? rank : end;
output_length->set_dim_value((end - start) < 0 ? 0 : (end - start));

This fix ensures the documentation accurately reflects the actual behavior, making it consistent with how ONNX Runtime handles these edge cases.

Fixes #6862.

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@github-project-automation github-project-automation Bot moved this to In progress in PR Tracker Jul 16, 2025
Copilot AI and others added 2 commits July 16, 2025 17:16
@justinchuby
Initial exploration of Shape operator specification issue
81db9ca 
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
@justinchuby
Fix Shape operator specification: correct range bounds and document s…
aeed96a 
…tart > end behavior

Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Copilot AI changed the title [WIP] Shape spec issue with start and end bound clamping Fix Shape operator specification: correct range bounds and document start > end behavior Jul 16, 2025
Copilot AI requested a review from justinchuby July 16, 2025 17:25
@justinchuby
Delete =0.10.7
c5cbfb9 
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby justinchuby added this to the 1.19 milestone Jul 16, 2025
@justinchuby
Copy link
Copy Markdown
Member

justinchuby commented Jul 16, 2025

@gramalingam

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 53.75%. Comparing base (985f3cf) to head (e80c6d7).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
onnx/backend/test/case/node/shape.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7132      +/-   ##
==========================================
+ Coverage   53.70%   53.75%   +0.04%     
==========================================
  Files         508      508              
  Lines       32097    32098       +1     
  Branches     2969     2969              
==========================================
+ Hits        17238    17254      +16     
+ Misses      14061    14048      -13     
+ Partials      798      796       -2

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

@justinchuby justinchuby added topic: operator Issues related to ONNX operators auto update doc Generate md/proto files automatically using the CI pipeline labels Jul 16, 2025
@github-project-automation github-project-automation Bot moved this from In progress to Done in PR Tracker Jul 16, 2025
@justinchuby justinchuby reopened this Jul 16, 2025
@github-project-automation github-project-automation Bot moved this from Done to In progress in PR Tracker Jul 16, 2025
@justinchuby justinchuby marked this pull request as ready for review July 16, 2025 22:15
@justinchuby justinchuby requested a review from a team as a code owner July 16, 2025 22:15
justinchuby and others added 3 commits July 16, 2025 15:15
@justinchuby justinchuby removed the auto update doc Generate md/proto files automatically using the CI pipeline label Jul 16, 2025
@github-project-automation github-project-automation Bot moved this from In progress to Reviewer approved in PR Tracker Jul 16, 2025
@gramalingam gramalingam merged commit a0722c3 into main Jul 16, 2025
39 checks passed
@gramalingam gramalingam deleted the copilot/fix-6862 branch July 16, 2025 23:02
@github-project-automation github-project-automation Bot moved this from Reviewer approved to Done in PR Tracker Jul 16, 2025
lfopensource pushed a commit to lfopensource/onnx that referenced this pull request Jul 22, 2025
@justinchuby @lfopensource
Fix Shape operator specification: correct range bounds and document s…
fc8aa51 
…tart > end behavior (onnx#7132)

The Shape operator specification incorrectly stated that axes are
clamped to the range `[0, r-1]`, but since the `end` parameter is
exclusive in slicing operations, the correct range should be `[0, r]`.
Additionally, the spec was missing documentation for the `start > end`
case.

## Changes Made

### Documentation Fixes
- **Corrected range specification**: Changed from `[0, r-1]` to `[0, r]`
in all Shape operator documentation
- **Added start > end behavior**: Added "If start > end, the result will
be an empty shape." to clarify edge case behavior

### Affected Versions
Updated documentation for all Shape operator versions that support
`start` and `end` attributes:
- `Shape_ver24_doc` (current version)
- `Shape_ver23_doc`
- `Shape_ver19_doc`
- `Shape_ver15_doc`

Note: Older versions (ver13, ver1) don't have `start`/`end` attributes
so no changes were needed.

### Test Coverage
- Added test case `test_shape("_start_greater_than_end", x, start=2,
end=1)` to validate the documented behavior
- Verified that the reference implementation correctly returns an empty
array when `start > end`

## Implementation Verification

The underlying C++ implementation was already correct - it properly
clamps both `start` and `end` to `[0, rank]` and handles the `start >
end` case by returning an empty shape:

```cpp
start = (start < 0) ? 0 : (start > rank) ? rank : start;
end = (end < 0) ? 0 : (end > rank) ? rank : end;
output_length->set_dim_value((end - start) < 0 ? 0 : (end - start));
```

This fix ensures the documentation accurately reflects the actual
behavior, making it consistent with how ONNX Runtime handles these edge
cases.

Fixes onnx#6862.

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 Share your feedback on Copilot coding agent for the chance to win a
$200 gift card! Click
[here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to
start the survey.

---------

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: lfopensource <linglingfan.cnn@gmail.com>
lfopensource added a commit to lfopensource/onnx that referenced this pull request Jul 23, 2025
@lfopensource @justinchuby
Fix Shape operator specification: correct range bounds and document s…
5ddf91b 
…tart > end behavior (onnx#7132)

The Shape operator specification incorrectly stated that axes are
clamped to the range `[0, r-1]`, but since the `end` parameter is
exclusive in slicing operations, the correct range should be `[0, r]`.
Additionally, the spec was missing documentation for the `start > end`
case.

## Changes Made

### Documentation Fixes
- **Corrected range specification**: Changed from `[0, r-1]` to `[0, r]`
in all Shape operator documentation
- **Added start > end behavior**: Added "If start > end, the result will
be an empty shape." to clarify edge case behavior

### Affected Versions
Updated documentation for all Shape operator versions that support
`start` and `end` attributes:
- `Shape_ver24_doc` (current version)
- `Shape_ver23_doc`
- `Shape_ver19_doc`
- `Shape_ver15_doc`

Note: Older versions (ver13, ver1) don't have `start`/`end` attributes
so no changes were needed.

### Test Coverage
- Added test case `test_shape("_start_greater_than_end", x, start=2,
end=1)` to validate the documented behavior
- Verified that the reference implementation correctly returns an empty
array when `start > end`

## Implementation Verification

The underlying C++ implementation was already correct - it properly
clamps both `start` and `end` to `[0, rank]` and handles the `start >
end` case by returning an empty shape:

```cpp
start = (start < 0) ? 0 : (start > rank) ? rank : start;
end = (end < 0) ? 0 : (end > rank) ? rank : end;
output_length->set_dim_value((end - start) < 0 ? 0 : (end - start));
```

This fix ensures the documentation accurately reflects the actual
behavior, making it consistent with how ONNX Runtime handles these edge
cases.

Fixes onnx#6862.

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 Share your feedback on Copilot coding agent for the chance to win a
$200 gift card! Click
[here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to
start the survey.

---------

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: lfopensource <linglingfan.cnn@gmail.com>
lfopensource added a commit to lfopensource/onnx that referenced this pull request Jul 23, 2025
@lfopensource @justinchuby
Fix Shape operator specification: correct range bounds and document s…
6d02a30 
…tart > end behavior (onnx#7132)

The Shape operator specification incorrectly stated that axes are
clamped to the range `[0, r-1]`, but since the `end` parameter is
exclusive in slicing operations, the correct range should be `[0, r]`.
Additionally, the spec was missing documentation for the `start > end`
case.

## Changes Made

### Documentation Fixes
- **Corrected range specification**: Changed from `[0, r-1]` to `[0, r]`
in all Shape operator documentation
- **Added start > end behavior**: Added "If start > end, the result will
be an empty shape." to clarify edge case behavior

### Affected Versions
Updated documentation for all Shape operator versions that support
`start` and `end` attributes:
- `Shape_ver24_doc` (current version)
- `Shape_ver23_doc`
- `Shape_ver19_doc`
- `Shape_ver15_doc`

Note: Older versions (ver13, ver1) don't have `start`/`end` attributes
so no changes were needed.

### Test Coverage
- Added test case `test_shape("_start_greater_than_end", x, start=2,
end=1)` to validate the documented behavior
- Verified that the reference implementation correctly returns an empty
array when `start > end`

## Implementation Verification

The underlying C++ implementation was already correct - it properly
clamps both `start` and `end` to `[0, rank]` and handles the `start >
end` case by returning an empty shape:

```cpp
start = (start < 0) ? 0 : (start > rank) ? rank : start;
end = (end < 0) ? 0 : (end > rank) ? rank : end;
output_length->set_dim_value((end - start) < 0 ? 0 : (end - start));
```

This fix ensures the documentation accurately reflects the actual
behavior, making it consistent with how ONNX Runtime handles these edge
cases.

Fixes onnx#6862.

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 Share your feedback on Copilot coding agent for the chance to win a
$200 gift card! Click
[here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to
start the survey.

---------

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: lfopensource <linglingfan.cnn@gmail.com>
MagellaX pushed a commit to MagellaX/onnx that referenced this pull request Aug 9, 2025
@justinchuby @MagellaX
Fix Shape operator specification: correct range bounds and document s…
8212fc9 
…tart > end behavior (onnx#7132)

The Shape operator specification incorrectly stated that axes are
clamped to the range `[0, r-1]`, but since the `end` parameter is
exclusive in slicing operations, the correct range should be `[0, r]`.
Additionally, the spec was missing documentation for the `start > end`
case.

## Changes Made

### Documentation Fixes
- **Corrected range specification**: Changed from `[0, r-1]` to `[0, r]`
in all Shape operator documentation
- **Added start > end behavior**: Added "If start > end, the result will
be an empty shape." to clarify edge case behavior

### Affected Versions
Updated documentation for all Shape operator versions that support
`start` and `end` attributes:
- `Shape_ver24_doc` (current version)
- `Shape_ver23_doc`
- `Shape_ver19_doc`
- `Shape_ver15_doc`

Note: Older versions (ver13, ver1) don't have `start`/`end` attributes
so no changes were needed.

### Test Coverage
- Added test case `test_shape("_start_greater_than_end", x, start=2,
end=1)` to validate the documented behavior
- Verified that the reference implementation correctly returns an empty
array when `start > end`

## Implementation Verification

The underlying C++ implementation was already correct - it properly
clamps both `start` and `end` to `[0, rank]` and handles the `start >
end` case by returning an empty shape:

```cpp
start = (start < 0) ? 0 : (start > rank) ? rank : start;
end = (end < 0) ? 0 : (end > rank) ? rank : end;
output_length->set_dim_value((end - start) < 0 ? 0 : (end - start));
```

This fix ensures the documentation accurately reflects the actual
behavior, making it consistent with how ONNX Runtime handles these edge
cases.

Fixes onnx#6862.

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 Share your feedback on Copilot coding agent for the chance to win a
$200 gift card! Click
[here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to
start the survey.

---------

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Yash solanki <alphacr792@gmail.com>
@osalbahr osalbahr mentioned this pull request Aug 27, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gramalingam gramalingam gramalingam approved these changes

+1 more reviewer

@justinchuby justinchuby justinchuby approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@justinchuby justinchuby

Copilot code review Copilot

Labels

topic: operator Issues related to ONNX operators

Projects

PR Tracker
Status: Done

Milestone

1.19

Development

Successfully merging this pull request may close these issues.

Shape spec issue with start and end bound clamping

3 participants

@justinchuby @gramalingam