-
Notifications
You must be signed in to change notification settings - Fork 150
Add __rtruediv__ and __rfloordiv__ to Scalar variables
#1701
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
Conversation
__rtruediv__ and __rdiv__ to _scalar_py_operators for reverse division support
ricardoV94
left a comment
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.
@copilot changes needed
__rtruediv__ and __rdiv__ to _scalar_py_operators for reverse division support__rtruediv__ and __rfloordiv__ to _scalar_py_operators for reverse division support
__rtruediv__ and __rfloordiv__ to _scalar_py_operators for reverse division support__rtruediv__ and __rfloordiv__ to Scalar variables
|
@copilot remove legacy |
Fixed in a4881ae. Removed legacy
Also removed unused |
__rtruediv__ and __rfloordiv__ to Scalar variables__rtruediv__ and __rfloordiv__ to Scalar variables and remove legacy __div__ operators
__rtruediv__ and __rfloordiv__ to Scalar variables and remove legacy __div__ operators__rtruediv__ and __rfloordiv__ to Scalar variables
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1701 +/- ##
==========================================
+ Coverage 81.67% 81.68% +0.01%
==========================================
Files 244 244
Lines 53558 53549 -9
Branches 9433 9433
==========================================
Hits 43741 43741
+ Misses 7337 7328 -9
Partials 2480 2480
🚀 New features to boost your workflow:
|
| y = 5.0 // x | ||
| assert isinstance(y.owner.op, IntDiv) | ||
| assert isinstance(y.type, ScalarType) | ||
| assert y.eval({x: 2.0}) == 2.0 |
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.
We don't cast to int here?
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 isn't testing the dtype, 2 == 2.0 is true in Python. I trust our old code was doing the correct thing already
Division of a constant by a
ScalarVariable(e.g.,1.0 / xor5.0 // x) was raisingTypeErrordue to missing reverse division operators.Changes
Scalar variables:
__rtruediv__method to_scalar_py_operatorsclass, delegating totrue_div(other, self)__rfloordiv__method to_scalar_py_operatorsclass, delegating toint_div(other, self)test_rtruedivto verify true division works correctlytest_rfloordivto verify floor division works correctlyTensor and XTensor variables:
__div__and__rdiv__methods from_tensor_py_operatorsclass inpytensor/tensor/variable.py__div__and__rdiv__methods fromXTensorVariableclass inpytensor/xtensor/type.pyIntegerDivisionErrorimportAll division operations now use Python 3's modern operators:
__truediv__,__floordiv__,__rtruediv__, and__rfloordiv__.Examples
The implementation follows the existing pattern of other reverse arithmetic operators (
__radd__,__rsub__,__rmul__,__rpow__).Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.