Skip to content
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

Merged

Conversation

lgoettgens
Copy link
Collaborator

Base.inv and AbstractAlgebra.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 and AbstractAlgebra.div differ. For this function, we need to either change the fallback of div! from AbstractAlgebra.div to Base.div, or change the docstring in a similar way as for inv!. Which of the two do we want here?

(In the any case, #1816 needs to be adjusted to be consistent.)

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.03%. Comparing base (11bbd58) to head (d7eb617).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/fundamental_interface.jl 50.00% 2 Missing ⚠️
src/Fields.jl 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@thofma
Copy link
Member

thofma commented Oct 1, 2024

Complicated. They need to fall back to the Base version, since this is what one should overload. But they should not fallback to the Base version for e.g. BigInt.

Edit: Nevermind. They should fallback to the AA.inv an AA.div version.

@thofma
Copy link
Member

thofma commented Oct 1, 2024

I am not sure a docstring inside AbstractAlgebra needs to use the AbstractAlgebra prefix.

@lgoettgens
Copy link
Collaborator Author

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

@lgoettgens lgoettgens marked this pull request as draft October 1, 2024 16:34
@@ -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.
Copy link
Member

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?

Copy link
Member

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.

@lgoettgens lgoettgens force-pushed the lg/mutating-fallbacks-correction branch from 3a54b66 to d7eb617 Compare October 2, 2024 09:01
@lgoettgens lgoettgens marked this pull request as ready for review October 2, 2024 09:02
the `AA.*` should only implement methods for julia types and delegate everything else
@lgoettgens
Copy link
Collaborator Author

I now adapted the docstrings of inv! and div!.
Furthermore, I encountered some methods that are attached to the wrong function. This shouldn't change any behavior due to the delegation from AA.* to Base.*, but makes it more consistent

function Base.div(f::PolyRingElem{T}, g::PolyRingElem{T}) where T <: RingElement
q, r = divrem(f, g)
return q
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

redundant due to

function Base.div(a::T, b::T) where T <: RingElem
return divrem(a, b)[1]
end

@fingolfin fingolfin merged commit 55c35d9 into Nemocas:master Oct 2, 2024
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants