Fixes for 0.6 #235

andreasnoack · 2017-02-12T18:50:39Z

The main parts are the fixes for the broadcasting and indexing which both were broken on Julia master. There are many fixes of deprecated syntax and I've changed some uses of the old ngenerate in favor of generated functions. I've decided to remove support for Julia version prior to 0.6 to make the source as simple as possible. The package has been undermaintained for a while so I think we should focus on making maintenance as easy as possible. Prior version fo Julia can just use old versions of this package.

@mbauman It would be appreciated if you could take a look at the indexing code. In particular, if it is really necessary with @generated in methods like https://github.com/JuliaStats/DataArrays.jl/blob/anj/06/src/indexing.jl#L164-L172 that used to be @ngenerated.

@simonster You wrote most of the code touched here so any comments from you would also be appreciated.

Rewrite broadcast as generated function to avoid world age problems. Use some of the broadcast functionality in Base. Update getindex and setindex to Base changes. Use generated functions instead of @nsplat. Use some of the Base machinery for getindex Fix many syntax deprecations

…rs.jl

deprecation warning

andreasnoack · 2017-02-12T18:52:49Z

Oh, and I can see that my build of Julia has become obsolete while finishing the PR.

ararslan · 2017-02-12T20:24:39Z

src/DataArrays.jl

@@ -1,14 +1,14 @@
-__precompile__()
+__precompile__(false)


What's keeping from being able to precompile?

ararslan · 2017-02-12T20:26:28Z

src/broadcast.jl

-                    $(Symbol("pool_$(k)")) = $(Symbol("A_$(k)")).pool
-                end : nothing
-            for k = 1:narrays]...))
+Base.map!{F}(f::F, B::Union{DataArray, PooledDataArray}, A0::AbstractArray, As::AbstractArray...) =


Is the typevar necessary here?

yes, otherwise it won't specialize on the function

Are you sure? I thought that was changed in Base a while back.

It's always possible I'm wrong 😃. But this is the situation described in JuliaLang/julia#19137, and there's no indication the behavior has changed there.

Ah, hmm. My bad then.

AFAIK, without the ::F, specialization will only happen if you call the function.

mbauman · 2017-02-12T20:30:08Z

src/indexing.jl

-    else
-        return pda.pool[getindex(pda.refs, I...)]
+
+@generated function Base.getindex(pda::PooledDataArray, I::Integer...)


I would just @inline this and the following.

ararslan · 2017-02-12T20:33:15Z

test/operators.jl

+                                  :(Base.lgamma),
+                                  :(Base.digamma),
+                                  :(Base.erf),
+                                  :(Base.erfc)]


No longer in Base (same with erf and digamma)

Add SpecialFunctions as dependency Use new type keywords

andreasnoack · 2017-02-12T22:39:41Z

Thanks for the comments. I've updated the code accordingly. The latest changes on master haven't made it into the Julia binary yet so we'll have to restart the tests when new binaries have been uploaded.

mbauman · 2017-02-13T03:22:11Z

src/indexing.jl

@@ -114,6 +114,8 @@ Base.getindex(t::AbstractDataArray, i::Real) =

 ## getindex: DataArray

+Base.linearindexing(x::Union{DataArray,PooledDataArray}) = Base.LinearFast()


This should be defined on the ::Type{<:Union{DataArray,PooledDataArray}}, IIRC.

ararslan · 2017-02-13T18:20:37Z

All of the remaining depwarns are from other packages. Does anything else need to happen here before this can be merged? LGTM.

andreasnoack · 2017-02-13T19:44:35Z

I think it is ready.

ararslan · 2017-02-13T19:57:00Z

Awesome. Unless there are further comments, I say we should merge this soon.

ararslan · 2017-02-13T21:19:43Z

src/DataArrays.jl

@@ -1,14 +1,14 @@
 __precompile__()

 module DataArrays
+    using Base: promote_op
    using Base.Cartesian, Compat, Reexport


Since Compat was removed from REQUIRE, it should be removed here too. (Not sure how this didn't cause CI to fail...)

True. I think it is installed by the dependencies.

Ah yeah, probably installed by StatsBase.

ararslan · 2017-02-13T21:22:03Z

src/DataArrays.jl

    using Base.Cartesian, Compat, Reexport
-    import Compat.String
    @reexport using StatsBase


This is tangential to the current changes, but it would make sense to me to stop reexporting StatsBase from this package. I'm not sure why we do it currently. If we do stop doing that, we could also drop the dependency on Reexport (not that it's a huge or problematic dependency, just saying).

nalimilan · 2017-02-13T21:36:55Z

test/broadcast.jl

@@ -20,7 +20,7 @@ rb = 1:5
 @test broadcast!(+, DataArray(Int, 2, 2), [1, 0], [1  4]) == [2 5; 1 4]
 @test broadcast!(+, DataArray(Int, 2), [1, 0], [1, 4]) == [2, 4]
 @test broadcast!(+, DataArray(Int, 2), [1, 0], 2) == [3, 2]
-@test broadcast!(abs, @data([-1, -2])) == @data([1, 2])
+# @test broadcast!(abs, @data([-1, -2])) == @data([1, 2])


Why is this commented out? Should it be removed altogether?

Yes. It should be removed. The in-place broadcast! is deprecated in Base.

andreasnoack · 2017-02-14T01:16:15Z

I've added Compat back. We need it for the @nsplat and @ngenerate macros. Eventually, we should just use @generated but I'll prefer to make that change in a separate PR.

Tests are failing because of a problem with the Julia binaries. I suggest that we just wait and see if tomorrow's binaries are more up to date.

ararslan · 2017-02-14T01:32:14Z

test/reducedim.jl

@@ -109,7 +109,7 @@ macro test_da_approx_eq(da1, da2)
        v2 = $(esc(da2))
        na = isna(v1)
        @test na == isna(v2)
-        defined = !na
+        defined = (!).(na)


.!na should work here as you've written it elsewhere, right?

Nope. Apparently, this form is required in macros. I tried the other version first.

That's... bizarre. Maybe file a Base issue?

ararslan · 2017-02-14T15:32:00Z

src/natype.jl

@@ -14,24 +14,28 @@
 ##
 ##############################################################################

-type NAtype
+struct NAtype


ERROR: LoadError: LoadError: syntax: extra token "NAtype" after end of expression Stacktrace: [1] include_from_node1(::String) at ./loading.jl:539 [2] include(::String) at ./sysimg.jl:14 [3] include_from_node1(::String) at ./loading.jl:539 [4] include(::String) at ./sysimg.jl:14 [5] anonymous at ./<missing>:2 while loading /home/travis/.julia/v0.6/DataArrays/src/natype.jl, in expression starting on line 17

??????

For some reason, the nightly build used an older commit than the previous one and therefore it doesn't include the struct changes. We can restart again when new binaries are up.

Looks like this is still happening with the nightly binaries. 😕

See https://status.julialang.org/. They are old. There is a problem on master that prevents the buildbot from making new ones.

ararslan · 2017-02-16T01:49:55Z

src/broadcast.jl

                $(Expr(:block, [
-                    :(@inbounds $(Symbol("v_$(k)")) = @nref $nd $(Symbol("A_$(k)")) d->$(Symbol("j_$(k)_d")))
-                    for k = find(t -> !(t <: DataArray || t <: PooledDataArray), As)]...))
+                    As[k] <: AbstractArray  && !(As[k] <: Union{DataArray, PooledDataArray}) ? quote


I think you could do AbstractDataArray instead of the Union there.

andreasnoack · 2017-02-17T20:03:37Z

I'll suggest that I test this one locally against latest Julia master and merge if the tests pass since we don't know when we'll have new nightlies for Travis testing.

ararslan · 2017-02-17T20:07:31Z

Sounds like a good plan to me!

ararslan · 2017-02-17T23:02:07Z

Thanks so much for your hard work here, Andreas!

nalimilan · 2017-02-19T21:39:09Z

The use of struct makes the tests fail. Better use the old type declaration syntax for now.

ararslan · 2017-02-19T21:55:16Z

It makes the tests fail because for some reason the nightly binaries are stuck at a commit before the Base change due to issues with the buildbots. Hopefully that should be fixed soon.

nalimilan · 2017-02-19T22:19:15Z

OK. But I guess it won't fix the tests on Julia 0.5, will it?

ararslan · 2017-02-19T22:24:57Z

Andreas bumped the minimum requirement for the package to 0.6-, so it won't work on 0.5 anyway.

andreasnoack added 5 commits February 10, 2017 16:45

Remove Compat code

3041382

Fix broadcast warnings and remove unused operator groups form operato…

6fe6e13

…rs.jl

Use iteration instead of indexing into indices in setindex to avoid

1c423b9

deprecation warning

Adjust CI

af188c7

ararslan requested a review from simonster February 12, 2017 20:23

ararslan reviewed Feb 12, 2017

View reviewed changes

src/DataArrays.jl Outdated

@@ -1,14 +1,14 @@

__precompile__()

__precompile__(false)

Copy link

Member

ararslan Feb 12, 2017

Choose a reason for hiding this comment

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

What's keeping from being able to precompile?

ararslan reviewed Feb 12, 2017

View reviewed changes

mbauman reviewed Feb 12, 2017

View reviewed changes

ararslan reviewed Feb 12, 2017

View reviewed changes

andreasnoack added 2 commits February 12, 2017 17:21

Reenable precompilation

4dbce68

Add SpecialFunctions as dependency Use new type keywords

Use @inline instead of @generate when possible

a2bb139

Now, erf, erfc, and digamma are in SpecialFunctions instead of Base

1416606

andreasnoack force-pushed the anj/06 branch from 9469c76 to 1416606 Compare February 12, 2017 22:42

Intialize arrays with all NAs. Flip bits _unsafe_getindex.

0c9184c

mbauman reviewed Feb 13, 2017

View reviewed changes

Define linearindexing for Type{T} instead of T

a38a6f6

ararslan reviewed Feb 13, 2017

View reviewed changes

nalimilan reviewed Feb 13, 2017

View reviewed changes

andreasnoack added 2 commits February 13, 2017 19:39

Update ! -> .! and remove commented out tests

a75d440

Reenable Compat. Necessary for old @nsplat and @ngenerate macros

ed30192

ararslan reviewed Feb 14, 2017

View reviewed changes

andreasnoack mentioned this pull request Feb 14, 2017

codegen support for efficient union representations JuliaLang/julia#20593

Merged

ararslan reviewed Feb 14, 2017

View reviewed changes

ararslan reviewed Feb 16, 2017

View reviewed changes

Make broadcast work for scalar Strings

937d396

andreasnoack force-pushed the anj/06 branch from 5756902 to 937d396 Compare February 16, 2017 01:57

Update syntax for indexing traits

9f68def

andreasnoack merged commit 2a83487 into master Feb 17, 2017

andreasnoack deleted the anj/06 branch February 17, 2017 22:31

		@@ -114,6 +114,8 @@ Base.getindex(t::AbstractDataArray, i::Real) =

		## getindex: DataArray

		Base.linearindexing(x::Union{DataArray,PooledDataArray}) = Base.LinearFast()

Fixes for 0.6 #235

Fixes for 0.6 #235

Conversation

andreasnoack commented Feb 12, 2017

andreasnoack commented Feb 12, 2017

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

simonster Feb 12, 2017 • edited Loading

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

andreasnoack commented Feb 12, 2017

Choose a reason for hiding this comment

ararslan commented Feb 13, 2017

andreasnoack commented Feb 13, 2017

ararslan commented Feb 13, 2017

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

andreasnoack commented Feb 14, 2017

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

andreasnoack commented Feb 17, 2017

ararslan commented Feb 17, 2017

ararslan commented Feb 17, 2017

nalimilan commented Feb 19, 2017

ararslan commented Feb 19, 2017

nalimilan commented Feb 19, 2017

ararslan commented Feb 19, 2017

simonster Feb 12, 2017 •

edited

Loading