StatsBase.jl icon indicating copy to clipboard operation
StatsBase.jl copied to clipboard

Aqua.jl found ambiguities

Open JeffreySarnoff opened this issue 2 years ago • 6 comments

5 ambiguities found

Ambiguity #1
==(x::Union{StatsBase.PValue, StatsBase.TestStat}, y::Real) @ StatsBase C:\Users\MrJSa\.julia\packages\StatsBase\XgjIN\src\statmodels.jl:90
==(x::Real, y::AbstractIrrational) @ Base irrationals.jl:90
Possible fix, define
  ==(::Union{StatsBase.PValue, StatsBase.TestStat}, ::AbstractIrrational)

Ambiguity #2
==(y::Real, x::Union{StatsBase.PValue, StatsBase.TestStat}) @ StatsBase C:\Users\MrJSa\.julia\packages\StatsBase\XgjIN\src\statmodels.jl:91
==(x::AbstractIrrational, y::Real) @ Base irrationals.jl:89

Possible fix, define
  ==(::AbstractIrrational, ::Union{StatsBase.PValue, StatsBase.TestStat})
Ambiguity #3
StatsBase.TestStat(v) @ StatsBase C:\Users\MrJSa\.julia\packages\StatsBase\XgjIN\src\statmodels.jl:80
(::Type{T})(x::AbstractChar) where T<:Union{AbstractChar, Number} @ Base char.jl:50

Possible fix, define
  StatsBase.TestStat(::AbstractChar)

Ambiguity #4
StatsBase.TestStat(v) @ StatsBase C:\Users\MrJSa\.julia\packages\StatsBase\XgjIN\src\statmodels.jl:80
(::Type{T})(x::Base.TwicePrecision) where T<:Number @ Base twiceprecision.jl:267

Possible fix, define
  StatsBase.TestStat(::Base.TwicePrecision)

Ambiguity #5
StatsBase.TestStat(v) @ StatsBase C:\Users\MrJSa\.julia\packages\StatsBase\XgjIN\src\statmodels.jl:80
(::Type{T})(z::Complex) where T<:Real @ Base complex.jl:44

Possible fix, define
  StatsBase.TestStat(::Complex)

JeffreySarnoff avatar May 02 '23 03:05 JeffreySarnoff

Thanks for the report. Luckily, I can't imagine that any of those ambiguities would be encountered in practice.

ararslan avatar May 12 '23 16:05 ararslan

Should these types be replaced with functions such as print_teststat and print_pvalue or at least not be a subtype of Real? Based on JuliaHub (https://juliahub.com/ui/Search?q=TestStat&type=code&w=true and https://juliahub.com/ui/Search?q=PValue&type=code&w=true) these are mainly used for printing.

devmotion avatar May 12 '23 20:05 devmotion

I don't know why they subtype Real, perhaps that part isn't necessary, but having them as types does permit composition in a way that separate functions wouldn't. For example, interpolating into a string would require wrapping like sprint(print_pvalue, p) whereas defining a method allows that to Just Work™.

ararslan avatar May 13 '23 00:05 ararslan

Yes, my impression was as well that these types are more convenient and composable than separate functions, ans hence removing the supertype and the comparison operators might be the preferable solution.

devmotion avatar May 13 '23 13:05 devmotion

See https://github.com/JuliaStats/StatsBase.jl/pull/668 for the history.

Is there any actual issue to fix here? I don't think the recommended policy is to fix all ambiguities when they don't cause actual problems, right?

nalimilan avatar Jun 17 '23 11:06 nalimilan

That said, it would be easy to fix the first series of ambiguities by defining == separately for PValue and TestStat, which is clear anyway.

nalimilan avatar Jun 17 '23 11:06 nalimilan