In dynamo tracing for index() use None as the default indicator for end and not -1 #119151
Conversation
…nd and not -1 Summary: In dynamo tracing, `index()`'s implementation currently has the default begin index as `0` and the default end index as`-1` which means that by default we're dropping the last element. Rather we should be doing `None` which will ensure that the last element is also checked. Test Plan: CI Differential Revision: D53392287
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/119151
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 3ddf5c6 with merge base 052e824 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D53392287 |
There was a problem hiding this comment.
It looks correct to me, but will @yanboliang sign off since I'm not quite sure what this polyfill file is supposed to do...
|
FYI, you can add a unit test after this: pytorch/test/dynamo/test_repros.py Line 3272 in b964a12 |
|
@pytorchbot merge -f 'Landed internally' (Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally) |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
) D53392287 botched the export somehow and the exported PR #119151 didn't contain the added test. The discrepancy is showing up on diff train patch up diff D53694548 Pull Request resolved: #119854 Approved by: https://github.com/kit1980, https://github.com/malfet
…nd and not -1 (#119151) Summary: In dynamo tracing, `index()`'s implementation currently has the default begin index as `0` and the default end index as`-1` which means that by default we're dropping the last element. Rather we should be doing `None` which will ensure that the last element is also checked. Test Plan: CI Differential Revision: D53392287 Pull Request resolved: #119151 Approved by: https://github.com/yanboliang
5 participants
Summary: In dynamo tracing,
index()'s implementation currently has the default begin index as0and the default end index as-1which means that by default we're dropping the last element. Rather we should be doingNonewhich will ensure that the last element is also checked.Test Plan: CI
Differential Revision: D53392287
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng