Skip to content

WIP Progress on incorporating StatsBase into statistics. #31

Closed
pdeffebach wants to merge 5 commits intoJuliaStats:nl/weightedstatsfrom
pdeffebach:peter_weighted_stats
Closed

WIP Progress on incorporating StatsBase into statistics. #31
pdeffebach wants to merge 5 commits intoJuliaStats:nl/weightedstatsfrom
pdeffebach:peter_weighted_stats

Conversation

@pdeffebach
Copy link

@pdeffebach pdeffebach commented Mar 18, 2020

No description provided.

nalimilan and others added 4 commits October 23, 2019 21:40
@nalimilan
Fix Travis and AppVeyor
8c3fb03 
Take inspiration from Pkg.jl to ensure the package is loaded
rather than the Statistics stdlib module.
Test Julia 1.2 and 1.3 (tests fail on 1.0)
@pdeffebach
Test
b528e79
@pdeffebach
incorporate scalarstats
348d6c0
@pdeffebach
small fix
9a0b645
@pdeffebach
Copy link
Author

pdeffebach commented Mar 29, 2020

@nalimilan this is ready for a review. My impression is that not everything has to be 100% perfect for this PR since it's being merged into your PR, and I would bet rebasing there is a high cost of keeping everything up to date.

The quantile function that returns a Vector of the quantiles is moved into Statistics, as is percentile. Everything else is the same.

Thanks!

nalimilan
nalimilan reviewed Mar 30, 2020
View reviewed changes
src/Statistics.jl
@@ -1,5 +1,4 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

Copy link
Member

@nalimilan nalimilan Mar 30, 2020

Choose a reason for hiding this comment

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

Revert this.

src/scalarstats.jl Outdated
#
#############################

"""
Copy link
Member

@nalimilan nalimilan Mar 30, 2020

Choose a reason for hiding this comment

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

Maybe keep these here for now. That reduces the diff, which makes things less messy, and we may want to move quantile to this file (or a separate file) later.

Copy link
Author

@pdeffebach pdeffebach Apr 11, 2020

Choose a reason for hiding this comment

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

I tried to do this but can't since this extends methods in statistics and we include scalarstats.jl at the top.

Copy link
Member

@nalimilan nalimilan May 27, 2020

Choose a reason for hiding this comment

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

OK. Then remove the heading above.

Project.toml Outdated
@@ -1,8 +1,8 @@
name = "Statistics"
uuid = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"
Copy link
Member

@nalimilan nalimilan Mar 30, 2020

Choose a reason for hiding this comment

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

I think you need to keep this to get CI to pass. We'll remove it later.

Copy link
Author

@pdeffebach pdeffebach Apr 11, 2020

Choose a reason for hiding this comment

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

I need to delete it in order for a Revise-based workflow to work, see my discourse question here.

@pdeffebach
Copy link
Author

pdeffebach commented Apr 11, 2020

Sorry I couldn't give better feedback, this is tough.

Not all tests pass. The scalarstats ones, including the quantile and percentile tests pass. But none of the tests relating to weighted sum and weighted mean pass. At the top of wsum.jl there is the if statement

# Weighted sum is only supported in Julia 1.4 and above
if VERSION >= v"1.4.0"
    _sum = sum
    _sum! = sum!
else
# all the code

So it's no surprise that isn't working. Though I don't understand what's going on. Can you help?

@nalimilan
Copy link
Member

nalimilan commented May 27, 2020

Sorry I missed your question. The VERSION check is incorrect as I wrote it before 1.4 was released, thinking that JuliaLang/julia#33310 would be included in that release. So you can just remove it for now and use the code from the other branch.

@musm
Copy link
Contributor

musm commented Nov 6, 2020

Isn't this PR missing a large portion of StatsBase ? If I'm not mistaken wasn't the goal to incorporate StatsBase into Statistics?

@nalimilan
Copy link
Member

nalimilan commented Nov 7, 2020

It's marked as WIP. And see also https://github.com/JuliaLang/Statistics.jl/pull/2.

@nalimilan nalimilan mentioned this pull request Feb 18, 2021
Closed
@nalimilan nalimilan force-pushed the nl/weightedstats branch 2 times, most recently from 1277ff9 to 71cde77 Compare September 25, 2021 15:38
@nalimilan
Copy link
Member

nalimilan commented Sep 25, 2021

I've moved the changes from this PR to https://github.com/JuliaLang/Statistics.jl/pull/2.

@nalimilan nalimilan closed this Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nalimilan nalimilan nalimilan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pdeffebach @nalimilan @musm