-
Notifications
You must be signed in to change notification settings - Fork 63
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
Correct mutating op fallbacks #1823
Correct mutating op fallbacks #1823
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1823 +/- ##
=======================================
Coverage 88.03% 88.03%
=======================================
Files 119 119
Lines 29993 29985 -8
=======================================
- Hits 26403 26397 -6
+ Misses 3590 3588 -2 ☔ View full report in Codecov by Sentry. |
Edit: Nevermind. They should fallback to the |
I am not sure a docstring inside AbstractAlgebra needs to use the AbstractAlgebra prefix. |
Including this docstring into Oscar should prefix. I'll extend this a bit and maybe explain the difference there |
@@ -328,7 +328,7 @@ neg!(a) = neg!(a, a) | |||
inv!(z, a) | |||
inv!(a) | |||
|
|||
Return `inv(a)`, possibly modifying the object `z` in the process. | |||
Return `AbstractAlgebra.inv(a)`, possibly modifying the object `z` in the process. |
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.
It can be very surprising (esp. for a Julia beginner) that AbstractAlgebra.inv != Base.inv
so I think it is good to prefix it here explicitly to raise the awareness. Even more so for when this appears in the Oscar manual.
Should the same be done for div
?
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.
Ah I see you wrote that as a question. So yeah, I think div!
should fallback to AA.div!
(not Base.div!
) and then its docstring should also be adjusted.
3a54b66
to
d7eb617
Compare
the `AA.*` should only implement methods for julia types and delegate everything else
I now adapted the docstrings of |
function Base.div(f::PolyRingElem{T}, g::PolyRingElem{T}) where T <: RingElement | ||
q, r = divrem(f, g) | ||
return q | ||
end |
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.
redundant due to
AbstractAlgebra.jl/src/algorithms/GenericFunctions.jl
Lines 77 to 79 in c70d3b6
function Base.div(a::T, b::T) where T <: RingElem | |
return divrem(a, b)[1] | |
end |
Base.inv
andAbstractAlgebra.inv
differ in handling Integers. The base version returns floats, while the AA version errors if it doesn't get a unit.Similarly,
Base.div
andAbstractAlgebra.div
differ. For this function, we need to either change the fallback ofdiv!
fromAbstractAlgebra.div
toBase.div
, or change the docstring in a similar way as forinv!
. Which of the two do we want here?(In the any case, #1816 needs to be adjusted to be consistent.)