define divrem for FieldElement instead of FieldElem

[heiderich]

I also added a test in relation to #249.

[wbhart]

This is not quite enough. We need to ensure AbstractAlgebra does not export the definition, otherwise it is type piracy (defining a function that Julia already defines, for Julia types).

Besides this, the Julia definition is probably more specific than this, so I doubt it will work anyway.

We explicitly need to define it for Rationals in src/julia/Rational.jl.

Again, you have to look at how we handled sqrt and exp. It's quite involved. It's best to grep for every instance of one of these to see all that needs to be done.

[heiderich]

This is not quite enough. We need to ensure AbstractAlgebra does not export the definition, otherwise it is type piracy (defining a function that Julia already defines, for Julia types).

Indeed:

julia> divrem(1//1,2//1)
(0, 1//1)

julia> import AbstractAlgebra

julia> divrem(1//1,2//1)
(1//2, 0//1)

This is what you want to avoid, right?

Besides this, the Julia definition is probably more specific than this, so I doubt it will work anyway.

What exactly do you mean will not work?

We explicitly need to define it for Rationals in src/julia/Rational.jl.

Again, you have to look at how we handled sqrt and exp. It's quite involved. It's best to grep for every instance of one of these to see all that needs to be done.

I tried to mimic this approach in the new commits. Now we get:

julia> divrem(1//1, 2//1)
(0, 1//1)

julia> using AbstractAlgebra

julia> divrem(1//1, 2//1)
(0, 1//1)

julia> AbstractAlgebra.divrem(1//1, 2//1)
(1//2, 0)

julia> R,(x,y) = AbstractAlgebra.PolynomialRing(AbstractAlgebra.QQ, ["x", "y"])
(Multivariate Polynomial Ring in x, y over Rationals, AbstractAlgebra.Generic.MPoly{Rational{BigInt}}[x, y])

julia> divrem(x,2*x)
(1//2, 0//1)

julia> R,(x,y) = AbstractAlgebra.PolynomialRing(AbstractAlgebra.RealField, ["x", "y"])
(Multivariate Polynomial Ring in x, y over Floats, AbstractAlgebra.Generic.MPoly{BigFloat}[x, y])

julia> divrem(x,2*x)
(0.50, 0.0)

[wbhart]

I think you've mostly done this right.

I think you should instead define divrem in AbstractAlgebra.jl for any type T to call Base.divrem. This will be the fallback (inside AbstractAlgebra).

Then you won't have to define it specifically for BigFloat and Integers and so on, as it will fall back to the Julia definition.

Also, is it really necessary to call AbstractAlgebra.divrem from inside Generic? If so, then we would need to use AbstractAlgebra.divrem everywhere in Generic, not just in this one place.

And everywhere we define a function divrem that is not for Julia types, we will have to define Base.divrem not just divrem.

[wbhart]

Oh, I just noticed, you did define divrem for all types T in AbstractAlgebra.jl.

So why was it then necessary to define divrem for Int and BigFloat in the julia directory?

[heiderich]

I think you should instead define divrem in AbstractAlgebra.jl for any type T to call Base.divrem. This will be the fallback (inside AbstractAlgebra).

Then you won't have to define it specifically for BigFloat and Integers and so on, as it will fall back to the Julia definition.

As I understand it,

function divrem(a::T, b::T) where T
  return Base.divrem(a, b)
end

in src/AbstractAlgebra.jl ensures that the standard divrem is used as a fallback.
We do not want this in some situations (such as in the context of issue #249).
Therefore the functions

function divrem(a::BigFloat, b::BigFloat)

in src/julia/Float.jl and

function divrem(a::Rational{T}, b::Rational{T}) where T <: Integer

in src/julia/Rational.jl are the implementations that we actually desire to use in order to fix #249.

So I guess we need both if we want access to both, the standard divrem and to the new functions.

An alternative would be to implement the divrem in AbstractAlgebra.jl as the other two functions in src/julia/Float.jl and src/julia/Rational.jl. But this is not what you did with sqrt. Following your suggestion I tried to mimic the implementation of sqrt.

Also, is it really necessary to call AbstractAlgebra.divrem from inside Generic? If so, then we would need to use AbstractAlgebra.divrem everywhere in Generic, not just in this one place.

Without this the fallback is used, which we do not want.

And everywhere we define a function divrem that is not for Julia types, we will have to define Base.divrem not just divrem.

I guess it depends on what one wants in the specific situation. But yes, there migh be instances. The aim of this PR is first to fix #249. As the fallback is used at least I do not expect this PR to cause any regressions.

[wbhart]

Ok, I didn't realise you intended to replace the Julia definitions of divrem for Float and BigFloat inside AbstractAlgebra. That is fine of course.

As for the second part of your comment, it is necessary to call AbstractAlgebra.divrem everywhere. Otherwise we will end up with inconsistent results from computations, depending on which version was used.

Also, my final comment still applies. Everywhere we define a divrem in AbstractAlgebra (for one of our types) we now need to overload Base.divrem, instead of just divrem.

[heiderich]

As for the second part of your comment, it is necessary to call AbstractAlgebra.divrem everywhere. Otherwise we will end up with inconsistent results from computations, depending on which version was used.

I changed this in a few places where I think it is appropriate. However I am not sure whether these functions are tested. Are there further places where changes are necessary?

Also, my final comment still applies. Everywhere we define a divrem in AbstractAlgebra (for one of our types) we now need to overload Base.divrem, instead of just divrem.

I do not quite understand why.

[wbhart]

If you don't put Base.divrem you are only defining divrem inside AbstractAlgebra, which means the definition is no longer available to users of AbstractAlgebra.

I'm surprised the test code doesn't fail actually. The test code is not inside but outside, so any time it calls divrem for such a case, it should fail.

[heiderich]

I though this is usually accomplished using "export". But we do not export "divrem". I am actually also surprised.

[wbhart]

Base.divrem is already "exported", which is why overloading it has the same effect (for those definitions).

[heiderich]

@wbhart So divrem seems to be available even though we do not know why. Do you think it is still necessary to overload Base.divrem?

[wbhart]

I don't really know.

[wbhart]

Ok, I'll merge this, for now. I'm sure we'll know soon enough if it is broken.