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

Allow for quantile to operate over an entire Matrix #108

Merged
merged 9 commits into from
Jun 9, 2022

Conversation

this-josh
Copy link
Contributor

As discussed here if you pass a matrix to quantile you get an error

julia> quantile(reshape(collect(1:100), (10, 10)), [0.00, 0.25, 0.50, 0.75, 1.00])
ERROR: MethodError: no method matching quantile!(::Matrix{Int64}, ::Vector{Float64}; sorted=false, alpha=1.0, beta=1.0)
Closest candidates are:
  quantile!(::AbstractArray, ::AbstractVector, ::AbstractArray; sorted, alpha, beta) at /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Statistics/src/Statistics.jl:936
  quantile!(::AbstractVector, ::Union{Tuple{Vararg{Real}}, AbstractArray}; sorted, alpha, beta) at /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Statistics/src/Statistics.jl:953
Stacktrace:
 [1] quantile(itr::Matrix{Int64}, p::Vector{Float64}; sorted::Bool, alpha::Float64, beta::Float64)
   @ Statistics /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Statistics/src/Statistics.jl:1070
 [2] quantile(itr::Matrix{Int64}, p::Vector{Float64})
   @ Statistics /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Statistics/src/Statistics.jl:1070
 [3] top-level scope
   @ REPL[2]:1

I made a small change which vectorises a matrix and provides the quantile for the whole matrix.

julia> quantile(reshape(collect(1:100), (10, 10)), [0.00, 0.25, 0.50, 0.75, 1.00])
5-element Vector{Float64}:
   1.0
  25.75
  50.5
  75.25
 100.0

This change was originally made in StatsBase with PR 773.

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #108 (4c8d6f1) into master (61a021b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   96.91%   96.93%   +0.01%     
==========================================
  Files           1        1              
  Lines         422      424       +2     
==========================================
+ Hits          409      411       +2     
  Misses         13       13              
Impacted Files Coverage Δ
src/Statistics.jl 96.93% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61a021b...4c8d6f1. Read the comment docs.

Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks!

src/Statistics.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
this-josh and others added 3 commits March 30, 2022 13:58
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Not sure what a meaningful way to work out the quantiles of a matrix inplace would look like, so remove the ! to make it clear it isn't inplace
Previous method caused type ambiguity, this is okay.
@nalimilan
Copy link
Member

Why did you remove the quantile! method? Isn't it need for quantile! to work on matrices?

Change from quantile to quantile!
Added a method which accepts q when determining quantiles over a matrix. This q is modified in place. Added a test to confirm this behaviour
@this-josh
Copy link
Contributor Author

Sorry your question about the in-place test confused me. Anyways, I think I've implemented the changes you requested.

test/runtests.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@61a021b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #108   +/-   ##
=========================================
  Coverage          ?   96.93%           
=========================================
  Files             ?        1           
  Lines             ?      424           
  Branches          ?        0           
=========================================
  Hits              ?      411           
  Misses            ?       13           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61a021b...847dff4. Read the comment docs.

@nalimilan nalimilan merged commit 576db0f into JuliaStats:master Jun 9, 2022
@nalimilan
Copy link
Member

Thanks, and sorry for the delay!

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