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

WIP Progress on incorporating StatsBase into statistics. #31

Closed
wants to merge 5 commits into from
Closed

WIP Progress on incorporating StatsBase into statistics. #31

wants to merge 5 commits into from

Conversation

pdeffebach
Copy link

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

@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 Outdated
@@ -1,5 +1,4 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

Copy link
Member

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
@@ -165,24 +165,6 @@ end
#
#############################

"""
Copy link
Member

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

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

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
@@ -1,8 +1,8 @@
name = "Statistics"
uuid = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"
Copy link
Member

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

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

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

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

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

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