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.

Sign up for GitHub

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

Jump to bottom

Aqua.jl found ambiguities #861

Open
JeffreySarnoff opened this issue May 2, 2023 · 6 comments · May be fixed by #866
Open

Aqua.jl found ambiguities #861

JeffreySarnoff opened this issue May 2, 2023 · 6 comments · May be fixed by #866
Labels
bug

Comments

@JeffreySarnoff
Copy link 

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)
@ararslan
Copy link
Member

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

@devmotion
Copy link
Member

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.

@ararslan
Copy link
Member

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™.

@devmotion
Copy link
Member

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.

@nalimilan
Copy link
Member

See #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
Copy link
Member

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

@devmotion devmotion linked a pull request Jun 17, 2023 that will close this issue
Open
@akirakyle akirakyle mentioned this issue Sep 14, 2023
Merged
@devmotion devmotion mentioned this issue Jan 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make PValue and TestStat non-Real + add tests with Aqua JuliaStats/StatsBase.jl
5 participants
@devmotion @nalimilan @JeffreySarnoff @ararslan @ParadaCarleton