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

Accept any iterable in scalar stats #472

Merged
merged 6 commits into from Apr 9, 2019
Merged

Accept any iterable in scalar stats #472

merged 6 commits into from Apr 9, 2019

Conversation

nalimilan
Copy link
Member

@nalimilan nalimilan commented Feb 28, 2019

Also improve a few docstrings.

Part of #449. Closes #342. Cc: @oxinabox

Also improve a few docstrings.
src/scalarstats.jl Outdated Show resolved Hide resolved
src/scalarstats.jl Outdated Show resolved Hide resolved
count = 1
value, state = y
y = iterate(x, state)
# Use Welford algorithm as seen in (among other places)
Copy link
Member Author

Choose a reason for hiding this comment

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

It's too bad I needed to copy the code from var, but if we move this to Statistics it will be easy to share the code in a common internal method.

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #472 into master will decrease coverage by 1.36%.
The diff coverage is 77.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
- Coverage   85.12%   83.76%   -1.37%     
==========================================
  Files          21       21              
  Lines        2098     2156      +58     
==========================================
+ Hits         1786     1806      +20     
- Misses        312      350      +38
Impacted Files Coverage Δ
src/moments.jl 87.42% <100%> (ø) ⬆️
src/scalarstats.jl 82.52% <71.87%> (-3.81%) ⬇️
src/hist.jl 86.93% <0%> (-6.08%) ⬇️
src/weights.jl 76.59% <0%> (-1.67%) ⬇️
src/toeplitzsolvers.jl 34% <0%> (-1.42%) ⬇️
src/counts.jl 89.04% <0%> (-0.62%) ⬇️
src/sampling.jl 86.52% <0%> (-0.55%) ⬇️
src/common.jl 0% <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 1823086...52d6e96. Read the comment docs.

src/scalarstats.jl Outdated Show resolved Hide resolved
src/scalarstats.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Contributor

A solid improvement on the state of the world

Make a copy when the element type is not concrete to ensure we choose an appropriate eltype.
@piever
Copy link
Contributor

piever commented Mar 23, 2019

Nice! Is this good to go? I think it's a really important improvement now that mean(skipmissing(v)) is the recommended way to compute summary statistics ignoring missing values.

function mean_and_var(A::RealArray, w::AbstractWeights; corrected::DepBool=nothing)
m = mean(A, w)
v = varm(A, w, m; corrected=depcheck(:mean_and_var, corrected))
function mean_and_var(x::RealArray, w::AbstractWeights; corrected::DepBool=nothing)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we still expect RealArray here? I thought this should also work with other arrays. E.g. Vector{Union{Missing, Float64}} as long as it does not contain missing (which will simply throw an error at run time if it does). Same questions for methods below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there are many other things to fix in this file. I'd rather fix them separately in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is OK to leave it for later. I reacted because you explicitly named this PR "Accept any iterable ..." 😄 so I thought this is exactly the change you want to make in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the title is a bit too ambitious.

@nalimilan
Copy link
Member Author

OK to merge then?

@nalimilan nalimilan merged commit b7fe44c into master Apr 9, 2019
@nalimilan nalimilan deleted the nl/itr branch April 9, 2019 16:12
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.

None yet

5 participants