move statistics.jl and related methods from Base to StatsBase #379

fredrikekre · 2018-05-20T00:53:43Z

This passes on Julia v0.6.2, master, and on fe/stats.

this commit removes cor, cov, median, median!, middle, quantile, quantile!, std, stdm, var, varm and linreg and moves them to StatsBase fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

codecov · 2018-05-20T01:16:33Z

Codecov Report

Merging #379 into master will decrease coverage by 9.79%.
The diff coverage is 7.14%.

@@            Coverage Diff            @@
##           master     #379     +/-   ##
=========================================
- Coverage   90.58%   80.78%   -9.8%     
=========================================
  Files          19       20      +1     
  Lines        2028     2274    +246     
=========================================
  Hits         1837     1837             
- Misses        191      437    +246

Impacted Files	Coverage Δ
src/StatsBase.jl	`100% <ø> (ø)`	⬆️
src/deprecates.jl	`0% <ø> (ø)`	⬆️
src/base.jl	`0% <0%> (ø)`
src/cov.jl	`97.05% <100%> (ø)`	⬆️
src/moments.jl	`99.4% <88.88%> (ø)`	⬆️

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 bb5138e...72896d2. Read the comment docs.

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

nalimilan · 2018-05-21T14:22:23Z

test/misc.jl

@@ -24,17 +24,17 @@ b = [true, false, false, true, false, true, true, false]

 # indicatormat

-I = [false true  false false false;
+II = [false true  false false false;
     true  false false false true;


Adjust alignment.

nalimilan · 2018-05-21T14:24:12Z

test/runtests.jl


+@static if StatsBase.BASESTATS_IN_STATSBASE
+    using Random
+    const Compatvar = var


Couldn't we just shadow the standard var, etc. functions instead of defining separate ones?

Since var and Compat.var are different functions on 0.6 I couldn't come up with a better solution. How did you mean?
I don't think it matters much, presumably StatsBase will drop 0.6 when 0.7 is released anyway, and then these will all just be var without Compat being involved.

I was thinking of something like const var = StatsBase.var in one branch, and const var = Compat.var in the other one. The point is precisely to limit as much as possible the changes that will have to be applied manually when dropping 0.6 support.

But

const var = Compat.var

would mean that StatsBase extend the unexported Compat.var and people will have to use a fully qualified name for this. In addition, that would mean StatsBase would export the unexported Compat.var and break things...

This is just a test file, it doesn't affect what users see at all AFAICT.

Ah, right, was thinking about the same case we have in the source file. But again, I don't think we can do that since they are two different functions, and we use them both.

OK, let's keep it that way then.

nalimilan · 2018-05-21T14:25:27Z

test/runtests.jl

@@ -22,6 +34,11 @@ tests = ["ambiguous",
         "statmodels"]#,
         #"statquiz"]

+@static if StatsBase.BASESTATS_IN_STATSBASE


Couldn't this be run even on 0.6? If needed, StatsBase can reexport Base functions.

Since the test in this file have been changed multiple times while it was in base I don't think so.

nalimilan · 2018-05-21T14:25:53Z

test/statistics.jl

+
+using Test, Random, LinearAlgebra, SparseArrays
+
+if true


Need to use StatsBase.BASESTATS_IN_STATSBASE?

nalimilan · 2018-05-25T14:08:20Z

I've added comments I had prepared based on the previous state of the PR. Some still appear to be relevant.

BTW, we should find a better file name than statistics.jl.

fredrikekre · 2018-05-25T14:13:07Z

BTW, we should find a better file name than statistics.jl.

Does it really matter what this is called? I assume that once 0.6 support is dropped we will just split this file and distribute the functions to the right place.

nalimilan · 2018-05-25T14:14:47Z

Maybe call it base.jl then?

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

nalimilan

Let's merge and tag soon since the functions have been removed from Base. Just needs to adjust the check based on the Julia version.

nalimilan · 2018-05-28T12:43:59Z

src/StatsBase.jl

@@ -193,11 +193,24 @@ module StatsBase
    export midpoints
 end

+const BASESTATS_IN_STATSBASE = false # VERSION >= v"1.2.3"


Need to update this now that PR has been merged in Julia?

ViralBShah · 2018-05-28T12:54:03Z

Note the coalesce to something rename when this is merged: JuliaLang/julia#27258

andreasnoack · 2018-05-28T14:12:54Z

@fredrikekre Is the version check updated and this ready to go?

fredrikekre · 2018-05-28T14:19:24Z

Yes.

nalimilan · 2018-05-28T15:20:02Z

Docstrings for new functions do not appear in the manual, but I guess that's just because we currently deploy it from Julia 0.6?

fredrikekre · 2018-05-28T16:33:45Z

Docstrings for new functions do not appear in the manual, but I guess that's just because we currently deploy it from Julia 0.6?

I actually didn't think about the docs, but yea, they don't appear in StatsBase on julia 0.6 at all so thats also why.

IMO the simplest thing for StatsBase would be to drop 0.6 at this point.

nalimilan · 2018-05-28T16:36:57Z

At least we could switch to deploying docs from 0.7, assuming it becomes a supported choice on Travis once the alpha is released.

martinholters · 2018-05-29T07:20:04Z

Um, this interacts badly with Compat. Compat provides its own var, std, ... with keyword support not present in 0.6, passing the call through to the respective function in Base. Which now doesn't work anymore on 0.7. But Compat cannot depend on StatsBase because StatsBase depends on Compat. So the solution would be to remove those functions from Compat, and let people use StatsBase on 0.6, too. But IIUC correctly, StatsBase only provides the old interface on 0.6, so that doesn't work. ~~Furthermore, StatsBase explicitly uses (at least) Compat.cov and Compat.varm. I wonder how that ever passes tests on a Julia build with those functions removed. Am I missing something?~~ EDIT: It only uses these Compat function on older Julia, where needed.

fredrikekre · 2018-05-29T07:25:25Z

Furthermore, StatsBase explicitly uses (at least) Compat.cov and Compat.varm.

I side stepped that, see #379 (comment)

martinholters · 2018-05-29T07:26:08Z

So, what I'd like to see is to have e.g. StatsBase.var have the same interface across Julia versions; then we can safely drop those from Compat.

fredrikekre · 2018-05-29T07:28:28Z

So, what I'd like to see is to have e.g. StatsBase.var have the same interface across Julia versions

Wouldn't that require StatsBase to have one exported var and one unexported var seems... impossible?

martinholters · 2018-05-29T07:32:08Z

My idea would be to export var after 0.7.0-DEV.5238 and only provide an unexported, new-interface version before that. Could that work?

martinholters · 2018-05-29T07:39:02Z

If that doesn't work, maybe a StatsBase.Compat submodule?

The two options I see is duplicating the code from here to Compat or moving the code from Compat here. Obviously, I'd favor moving code over duplicating it.

Well, a third option would be to create a new package StatsCompat or whatever just for those and move the Compat code there. But note that StatsBase could not depend on it, making adjustments here necessary anyway.

nalimilan · 2018-05-29T07:43:04Z

I agree with @martinholters. We should:

have StatsBase provide unexported var with the same interface on 0.6 and 0.7, but export it only after 0.7.0-DEV.5238
remove var from Compat after 0.7.0-DEV.5238, and have it just print a deprecation warning pointing to StatsBase (just like Base does)

fredrikekre · 2018-05-29T08:03:55Z

This would be very much simpler if we could have separate branches for pre and post 0.7.0-DEV.5238

martinholters · 2018-05-29T08:15:05Z

Ah, some of those functions had already been extended in StatsBase, right? Then having them suddenly be distinct, unexported functions seems like a no-go. I can try to prepare a PR, putting the code from Compat into a StatsBase.Compat submodule, if that is considered an option.

nalimilan · 2018-05-29T08:18:55Z

I guess it's fine to make different branches if that helps.

martinholters · 2018-05-29T08:22:25Z

I don't see how that would help much. Wouldn't we still need to provide e.g. two different varm functions on 0.6, the Base one with extensions (if I got that right) and an unexported one with the 0.7-interface?

fredrikekre · 2018-05-29T08:24:46Z

I can try to prepare a PR, putting the code from Compat into a StatsBase.Compat submodule, if that is considered an option.

That seems like the simplest approach, and direct Compat.var to StatsBase.Compat.var (although we need another name for that internal Compat module I think since StatsBase uses Compat for other things...)

I guess it's fine to make different branches if that helps.

I am trying this now, should be doable.

I don't see how that would help much.

Then we can do e.g. Base.var(args...) instead of import Base: var; function var .. end for adding methods, which lets us have a module local var and also extend the Base function.

martinholters · 2018-05-29T08:30:48Z

Of course! If that pans out, it would be better than the submodule approach, I think. Only if people had been doing StatsBase.var, they would be in for a surprise. But why would they? But are separate branches better than a lot of if VERSION blocks?

fredrikekre · 2018-05-29T08:48:35Z

After trying some I think that the submodule is much simpler, albeit uglier. I think most users have not upgraded yet anyway, and when they do they will likely drop 0.6 right away, meaning that this ugly middle ground might not be visible that much. Some other things that complicates this is that StatsBase have its own deprecations for some of these functions, and also, the methods that StatsBase extend does not use dims as a kwarg.

martinholters · 2018-05-29T09:07:12Z

After trying some I think that the submodule is much simpler, albeit uglier.

Are you working on it or should I give it a shot?

fredrikekre · 2018-05-29T09:21:54Z

#382

fix JuliaLang/julia#25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre mentioned this pull request May 20, 2018

move statistics out of Base JuliaLang/julia#27152

Merged

fredrikekre added a commit to JuliaLang/julia that referenced this pull request May 25, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

1aa4c99

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre added a commit to JuliaLang/julia that referenced this pull request May 25, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

f27a71d

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre added a commit to JuliaLang/julia that referenced this pull request May 25, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

fe2b611

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre force-pushed the fe/stats-from-base-to-statsbase branch from 8fe4df1 to 2c7f670 Compare May 25, 2018 13:31

nalimilan reviewed May 25, 2018

View reviewed changes

fredrikekre force-pushed the fe/stats-from-base-to-statsbase branch from 2c7f670 to 53d5024 Compare May 25, 2018 14:19

fredrikekre added a commit to JuliaLang/julia that referenced this pull request May 28, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

46a477a

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

nalimilan approved these changes May 28, 2018

View reviewed changes

move move cor, cov, std, stdm, var, varm and linreg to StatsBase

72896d2

fredrikekre force-pushed the fe/stats-from-base-to-statsbase branch from 53d5024 to 72896d2 Compare May 28, 2018 13:01

andreasnoack merged commit b7212f9 into JuliaStats:master May 28, 2018

fredrikekre deleted the fe/stats-from-base-to-statsbase branch May 28, 2018 16:32

martinholters mentioned this pull request May 29, 2018

Neither advertise the functions moved to StatsBase nor test them on 0.7 JuliaLang/Compat.jl#559

Merged

fredrikekre mentioned this pull request May 29, 2018

Tag StatsBase.jl v0.23.0 JuliaLang/METADATA.jl#14887

Closed

move statistics.jl and related methods from Base to StatsBase #379

move statistics.jl and related methods from Base to StatsBase #379

Conversation

fredrikekre commented May 20, 2018

codecov bot commented May 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented May 25, 2018

fredrikekre commented May 25, 2018

nalimilan commented May 25, 2018

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViralBShah commented May 28, 2018

andreasnoack commented May 28, 2018

fredrikekre commented May 28, 2018

nalimilan commented May 28, 2018

fredrikekre commented May 28, 2018

nalimilan commented May 28, 2018

martinholters commented May 29, 2018 • edited Loading

fredrikekre commented May 29, 2018

martinholters commented May 29, 2018

fredrikekre commented May 29, 2018

martinholters commented May 29, 2018

martinholters commented May 29, 2018

nalimilan commented May 29, 2018

fredrikekre commented May 29, 2018

martinholters commented May 29, 2018

nalimilan commented May 29, 2018

martinholters commented May 29, 2018

fredrikekre commented May 29, 2018

martinholters commented May 29, 2018

fredrikekre commented May 29, 2018

martinholters commented May 29, 2018

fredrikekre commented May 29, 2018

codecov bot commented May 20, 2018 •

edited

Loading

martinholters commented May 29, 2018 •

edited

Loading