Skip to content

min/max support for SymInt/Floats, finish as_strided/scatter/squeeze() backward symint support#86609

Closed
bdhirsh wants to merge 14 commits intogh/bdhirsh/323/basefrom
gh/bdhirsh/323/head
Closed

min/max support for SymInt/Floats, finish as_strided/scatter/squeeze() backward symint support#86609
bdhirsh wants to merge 14 commits intogh/bdhirsh/323/basefrom
gh/bdhirsh/323/head

Conversation

@bdhirsh
Copy link
Collaborator

@bdhirsh bdhirsh commented Oct 10, 2022

This PR shouldn't matter too much, but I figured I'd land it instead of deleting. PySymInt.min/max are technically broken today, and this fixes them - but it doesn't matter (yet) because nobody is calling min() / max() on symints from python (they all happen using std::min/max in C++, which desugar to lt / gt calls).

Stack from ghstack (oldest at bottom):

…) backward symint support

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 10, 2022

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/86609

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 8bd5b66:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

…er/squeeze() backward symint support"

[ghstack-poisoned]
@albanD
Copy link
Collaborator

albanD commented Oct 11, 2022

This is already in master now

…er/squeeze() backward symint support"

[ghstack-poisoned]
@bdhirsh
Copy link
Collaborator Author

bdhirsh commented Oct 11, 2022

It looks like #86643 hasn't landed yet?

(Separately, seems like CI didn't kick off after I updated this PR for some reason)

…er/squeeze() backward symint support"

[ghstack-poisoned]
…er/squeeze() backward symint support"

[ghstack-poisoned]
if method in ["min", "max"]:
# op = getattr(builtins, method)
return self
op = getattr(builtins, method)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albanD I could delete this PR, but I noticed this - probably still worth landing?

…er/squeeze() backward symint support"

[ghstack-poisoned]
…er/squeeze() backward symint support"

[ghstack-poisoned]
…er/squeeze() backward symint support"

[ghstack-poisoned]
…er/squeeze() backward symint support"

[ghstack-poisoned]
…er/squeeze() backward symint support"

[ghstack-poisoned]
…er/squeeze() backward symint support"

[ghstack-poisoned]
…er/squeeze() backward symint support"

[ghstack-poisoned]
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

return py_type(out, self.shape_env)

def unary_magic_impl(self):
if method in ["ceil", "floor"]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This one doesn't need to move since it is only used in the if right? The comment can be removed though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep - i'll remove this

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 13, 2022
…er/squeeze() backward symint support"

This PR shouldn't matter too much, but I figured I'd land it instead of deleting. `PySymInt.min/max` are technically broken today, and this fixes them - but it doesn't matter (yet) because nobody is calling `min()` / `max()` on symints from python (they all happen using `std::min/max` in C++, which desugar to lt / gt calls).




[ghstack-poisoned]
…er/squeeze() backward symint support"

This PR shouldn't matter too much, but I figured I'd land it instead of deleting. `PySymInt.min/max` are technically broken today, and this fixes them - but it doesn't matter (yet) because nobody is calling `min()` / `max()` on symints from python (they all happen using `std::min/max` in C++, which desugar to lt / gt calls).




[ghstack-poisoned]
@facebook-github-bot facebook-github-bot deleted the gh/bdhirsh/323/head branch June 8, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request release notes: jit release notes category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants