Fix ambiguities #192
Fix ambiguities #192
Conversation
|
Now I have updated it so that all ambiguities other than the ones I think are false positives are fixed. Towards the end I however realized that there might be a better way to handle the constructors, more about the alternative solution below. False positivesIt currently reports the following issues As mentioned above I'm however not able to trigger any of these. In fact all of them have specific definitions already, but they seem to not get picked up by Aqua. We already have the methods Arf(x::Rational; prec::Integer = _precision(x)) = set!(Arf(; prec), x)
Arb(x::Rational; prec::Integer = _precision(x)) = set!(Arb(; prec), x)The warning related to those go away if they are instead defined as Arf(x::Rational{S}; prec::Integer = _precision(x)) where {S} = set!(Arf(; prec), x)
Arb(x::Rational{S}; prec::Integer = _precision(x)) where {S} = set!(Arb(; prec), x)But as fair as I know these are equivalent? Similarly we already have methods for the for T in [ArbSeries, AcbSeries]
@eval function Base.:+(p::$T, q::$T)
deg = _degree(p, q)
return add_series!($T(degree = deg, prec = _precision(p, q)), p, q, deg + 1)
end
@eval function Base.:*(p::$T, q::$T)
deg = _degree(p, q)
return mullow!($T(degree = deg, prec = _precision(p, q)), p, q, deg + 1)
end
end
function Base.:+(p::AcbSeries, q::ArbSeries)
deg = _degree(p, q)
res = AcbSeries(q, degree = deg, prec = _precision(p, q))
return add_series!(res, p, res, deg + 1)
end
function Base.:*(p::AcbSeries, q::ArbSeries)
deg = _degree(p, q)
res = AcbSeries(q, degree = deg, prec = _precision(p, q))
return mullow!(res, p, res, deg + 1)
endWhich should cover all cases. But Aqua doesn't seem to pick it up. Am I just missing something here or is this a problem with Aqua? I looked for any open issues on Aqua related to false positives, but couldn't find anything. Do you know anything about this @devmotion? Alternative solutionLet me take The need to define separate constructors for Arb(x; prec::Integer = _precision(x)) = set!(Arb(; prec), x)It cant know if this constructor should be used or the default one for e.g. Arb(
x::Union{Real,Tuple{<:Real,<:Real},NTuple{2,Union{MagLike,ArfLike,BigFloat}},MagLike,ArfLike,ArbLike};
prec::Integer = _precision(x),
) = set!(Arb(; prec), x)At least it passes all the tests. One benefit with specializing our constructor like this is that we don't have to write the code for construction from Do you have any preferences between the two alternatives @kalmarek? I'm currently leaning towards the latter one, i.e. adding the above type constraint to the |
Joel-Dahne
force-pushed
the
fix-ambiguities
branch
from
bb36223
to
b944755
Compare
|
@Joel-Dahne Congratulations on handing in the thesis! and thanks for picking this up so quickly! Honestly speaking I don't like the idea of changing our code style just to satisfy some tool ;) Looking at Aqua docs I see that it is possible to override the reported ambiguities for specific methods like this: ambiguities=(exclude=[Arblib.Arb], broken=true),what do you think about this? |
|
I agree, it is a lot of code to handle some edge cases that most likely no one will care about. I'll try that override and remove changes to the constructors I have made. I'll probably keep the constructor from |
Fixes two issues with the implementation of `Base._fast` and one issue with the tests. - It no longer has ambiguities for `Base._fast(::typeof(min), x::Arb, y::AbstractFloat)`. - It now also supports `ArbRef`, the previous version would give wrong results. - Fix the test for `Base._fast` not actually using an input for which it would otherwise fail. Also added tests for all changes.
It is no longer defined for `Mag`, since `Mag` is not an `AbstractFloat` it is not needed. It is now defined for `ArfRef` and `ArbRef`.
Similar to the methods in Base they throw an error for non-real input. For Mag, Arf and Arb they are defined for Complex and for Arb also for Acb.
Joel-Dahne
force-pushed
the
fix-ambiguities
branch
from
b944755
to
1edf991
Compare
|
Okay, here is a new version! It fixes the problematic ambiguities and adds a setter from Complex. It ignores a number of method ambiguities in the tests and I have added a detailed comment about why what we ignore and why. The false positives from Aqua we should however maybe report? As far as I can tell they are bugs? If they are fixed we can remove the ignore for |
The goal of this PR is to fix all the ambiguities reported by Aqua, see #186. It is not fully finished, there are still some ambiguities left to handle, hence the tests currently fails.
The remaining ones to handle are:
For the ones related to
Complexit should be straight forward to add, I just didn't have the time to finish it yet. I will probably add similar methods forAcbin that case as well.For the other ones I have, as mentioned in #186, not been able to trigger them. The reported issues are
But everything seems to work fine
Am I missing something here?