-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add symbolic singleton int #110370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add symbolic singleton int #110370
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/110370
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated FailuresAs of commit b566a52 with merge base f878633 ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
[ghstack-poisoned]
[ghstack-poisoned]
test/test_sympy_utils.py
Outdated
# ge | ||
self.assertTrue(is_ge(j1, sympy.Integer(1))) | ||
with self.assertRaisesRegex(ValueError, "indeterminate"): | ||
is_ge(j1, sympy.Integer(3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This being indeterminate is kind of interesting, it's possible we should just return False here. Well this is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a tricky one because sympy only allows overriding of Ge. The other relations (Lt, Gt, Le) are, by consequence, all derived from Ge e.g., Lt(a, b) := !Ge(a, b). This would mean if we define the "indeterminate" j1 >= 3, the also "indeterminate" j0 < 3 will be evaluated to be True!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah fine for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk if all the rules are right but whatever, we can fix them as we go
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
Separating this out so we can check perf more easily Pull Request resolved: #110371 Approved by: https://github.com/ezyang ghstack dependencies: #110044, #110369, #110370
@pytorchbot revert -m "bottom diff is causing a plethora of internal failures" -c ghfirst |
@pytorchbot successfully started a revert job. Check the current status here. |
@soulitzer your PR has been successfully reverted. |
This reverts commit a7145cb. Reverted #110370 on behalf of https://github.com/PaliC due to bottom diff is causing a plethora of internal failures ([comment](#110370 (comment)))
reland of #110370 [ghstack-poisoned]
reland of #110370 [ghstack-poisoned]
reland of #110370 [ghstack-poisoned]
reland of #110370 [ghstack-poisoned]
reland of #110370 Pull Request resolved: #110674 Approved by: https://github.com/ezyang ghstack dependencies: #110673
Stack from ghstack (oldest at bottom):