Skip to content

Conversation

sebimarkgraf
Copy link
Contributor

This adds a reproduction of the rule introduced in f42c2bb leading to an incorrect rewrite of the graph.

The original rule does not consider the step parameter, which can influence the result of a Slice to be the identity even when input and output shape are equivalent.

The potential fix seems to be to not apply the rule on step != 1, therefore the second commit adds this to the original rule implementation.

@sebimarkgraf
Copy link
Contributor Author

@microsoft-github-policy-service agree

@gramalingam
Copy link
Collaborator

Thanks for catching this!

Copy link

codecov bot commented Aug 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.08%. Comparing base (41c5dd5) to head (6584bcc).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2478      +/-   ##
==========================================
+ Coverage   70.06%   70.08%   +0.02%     
==========================================
  Files         212      212              
  Lines       25638    25646       +8     
  Branches     2572     2573       +1     
==========================================
+ Hits        17964    17975      +11     
+ Misses       6783     6781       -2     
+ Partials      891      890       -1     

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

@justinchuby justinchuby enabled auto-merge (squash) August 9, 2025 00:08
@github-project-automation github-project-automation bot moved this from Todo to Done in ONNX Script Review Board Aug 9, 2025
@justinchuby
Copy link
Collaborator

Could you format the code with lintrunner f?

auto-merge was automatically disabled August 13, 2025 09:47

Head branch was pushed to by a user without write access

@sebimarkgraf
Copy link
Contributor Author

Ran lintrunner f.
I am getting a Could not find a lintrunner config at: '.lintrunner.private.toml'., not sure if that is expected.

@justinchuby
Copy link
Collaborator

Ran lintrunner f. I am getting a Could not find a lintrunner config at: '.lintrunner.private.toml'., not sure if that is expected.

That’s expected. Thanks

@justinchuby justinchuby enabled auto-merge (squash) August 13, 2025 13:28
@justinchuby justinchuby merged commit 7407431 into microsoft:main Aug 13, 2025
24 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants