Defunctorize #15804

martinholters · 2016-04-08T14:03:46Z

Deprecate (almost) everything in functors.jl (see #15692) and also otherwise remove now-unnecessary functor constructs. Comments on the choice of what to deprecate and what to remove are especially welcome as I was rather unsure there!

Two functor-like constructs have been kept/introduced (Predicate and BitChunkFunctor) to convey additional information to dispatch upon. For Predicate this would be "function is pure and returns a Bool". Is there any way of wringing that out of Julia automatically, or are these tag types needed?

andreasnoack · 2016-04-08T14:56:00Z

Is there a point in keeping the content of functors.jl but stop using them in base for a single release cycle. In that way, packages can continue them instead of getting a huge slow down on 0.4 when they have to use functions.

KristofferC · 2016-04-08T14:58:31Z

Can't that be adressed with @compat?

ViralBShah · 2016-04-08T15:02:11Z

Nice net deletion of code.

martinholters · 2016-04-08T15:31:16Z

Like @compat * gives MulFun() or * depending on version?

StefanKarpinski · 2016-04-08T15:54:13Z

Excellent. I was hoping someone would take a crack at this (I've been wanting to myself).

StefanKarpinski · 2016-04-08T15:58:28Z

We do need to figure out a workable deprecation strategy.

andreasnoack · 2016-04-08T16:20:20Z

Can't that be addressed with @compat?

Maybe, but I think it would require an ~~AST~~ expression rewrite entry to Compat.jl and it might be a bit tricky to get right in term of figuring out which of the *s and + that should be replaced. We cannot use dispatch there since all the different functions are just Functions on 0.4 which was why I suggested that we waited a bit with removing the functors.

tkelman · 2016-04-08T18:13:21Z

base/bitarray.jl

-map!(f::Function, dest::BitArray, A::BitArray, B::BitArray) = map!(specialized_bitwise_binary(f), dest, A, B)
+map!(f::typeof(!), dest::BitArray, A::BitArray) = map!(~, dest, A)
+map!(f::typeof(zero), dest::BitArray, A::BitArray) = falses(dest)
+map!(f::typeof(one), dest::BitArray, A::BitArray) = trues(dest)


these should write into dest, not return a new array

Well spotted. And this also indicates that tests are missing...

yes, though it can be tricky to test that in place methods are actually in place, and we don't do it explicitly as often as we should

Interestingly, it did work as it was as falses(dest) and trues(dest) do change their argument. But I'll open a separate issue or PR for that and have changed the implementation here to call fill!.

Indeed, falses(::BitArray) and trues(::BitArray) are undocumented methods with a behavior very different from what one could expect. I'd say your current code is fine if we keep these methods. Better open an issue/PR to document them, or remove them altogether.

EDIT: by "remove", I actually meant "restrict the signature to a tuple or varargs of integers.

I was going to implement falses(::BitArray) and trues(::BitArray) to behave similar to zeros(::AbstractArray) and document that. EDIT: See #15827.

martinholters · 2016-04-08T18:53:07Z

How about a dedicated macro (perhaps in Compat) for conditional functorization to avoid the hassles involved in integrating this in @compat?

andreasnoack · 2016-04-08T19:04:16Z

How about a dedicated macro (perhaps in Compat) for conditional functorization to avoid the hassles involved in integrating this in @compat?

That seems like a much simpler solution. I.e. something like @functorize + that then translates into AddFun() in 0.4 and is a no-op in 0.5, right?

martinholters · 2016-04-08T19:17:15Z

@andreasnoack yes, that is exactly what I meant.

vtjnash · 2016-04-13T00:08:34Z

any idea which packages that would affect? another way to deal with this is to just skip the dep-warning (e.g. rely on backwards compatibility instead of upgrading and using Compat's forward compatibility).

or the packages that care can just branch and pin the current version for v0.4

martinholters · 2016-04-13T08:09:21Z

@vtjnash Assuming AddFun to be the most commonly used functor, https://github.com/search?utf8=%E2%9C%93&q=AddFun+user%3AJuliaPackageMirrors&type=Code&ref=searchresults yields:

ColorVectorSpace.jl
DataArrays.jl
DistributedArrays.jl
FixedPoint.jl
FixedPointNumbers.jl
FixedSizeArrays.jl
NullableArrays.jl
ParallelAccelerator.jl
SparseVectors.jl

I haven't checked where use of functors is actually performance critical and not just out of habit. But I assume even closer examination will leave enough affected packages to warrant a working deprecation strategy. Ideas so far:

Keep functors for now (defined as in the proposed deprecations), deprecate later (0.6?)
Provide a @functorize macro. (in Compat or a dedicated package?)

Opinions?

tkelman · 2016-04-13T08:12:35Z

I like deprecating now but double-checking that the packages still work correctly (at least in cases where they work on master without this PR), and going with something like @functorize in Compat for the sake of avoiding deprecation warnings for packages that want to maintain multi-Julia-version compatibility on the same branch.

martinholters · 2016-04-14T07:23:42Z

Rebased and updated to also remove functors introduced in #15585.

KristofferC · 2016-04-14T07:35:05Z

base/sparse/sparsematrix.jl


-function is_hermsym(A::SparseMatrixCSC, check::Func)
+function is_hermsym(A::SparseMatrixCSC, check::Function)


@vtjnash Did the following comment on my PR that changed the same thing as this one:

it would be better not to use ::Function in a signature, now that everything it callable, that distinction can be a bit fuzzy

#15696 (comment)

True, Callable (as an alias for Union{Function,DataType} that could easily be changed in the future) would probably be better, but Function is used all over the place in signatures. Maybe better handled in separate PR?

martinholters · 2016-04-14T08:53:25Z

Searching for MulFun, too, the list from above needs to be extended by AxisAlgorithms, PositiveFactorizations, and Interpolations. On the other hand, FixedPoint, NullableArrays, ParallelAccelerator, and SparseVectors don't work on master. This leaves the following packages:

AxisAlgorithms
ColorVectorSpace
DataArrays
DistributedArrays
FixedPointNumbers
FixedSizeArrays
Interpolations
PositiveFactorizations

Of those, the only one not working (as tested with Pkg.test) with this PR is DataArrays due to its use of CentralizedAbs2Fun which I have removed rather than deprecated. All the others test fine (except for heaps of deprecation warnings).

Edit: Somehow I missed that DataArray fails on master, too. But once that is fixed, the issue of CentralizedAbs2Fun remains, of course.

martinholters · 2016-04-18T09:33:52Z

Submitted a PR for @functorize to Compat (see JuliaLang/Compat.jl#184).

timholy · 2016-04-20T00:41:54Z

Looks really great. I agree we can deal with the signature issue separately; at least Func->Function is effectively a broadening. I don't see anything that needs to change; surprising rate of test failure, but it seems likely to be unrelated.

Sorry this sat long enough to pick up a merge conflict---there's a lot of (exciting) churn right now. Can I ask you to kindly rebase, and we'll see how the tests fare again?

martinholters · 2016-04-20T06:55:13Z

Ok, rebased. Also, I added a deprecation for CentralizedAbs2Fun (as it is used by DataArrays), but it looks a bit scary. Is there a better way to achieve this?

Also, I'm not at all happy with the deprecation output:

WARNING: Base.MulFun is deprecated, use Base.#* instead.

is bad enough. But

WARNING: Base.CentralizedAbs2Fun is deprecated, use Base.##557#558 instead.

is absolutely unacceptable. Is there any remedy short of tweaking the deprecation code in module.c?

martinholters · 2016-04-20T06:57:03Z

base/deprecated.jl

@@ -1057,6 +1057,8 @@ for (Fun, func) in [(:IdFun, :identity),
        (::Type{typeof($(func))})() = $(func)
    end
 end
+@deprecate_binding CentralizedAbs2Fun typeof(centralizedabs2fun(0)).name.primary


Can the type of the anonymous function instantiated in centralizedabs2fun somehow be determined without calling it with a dummy argument?

martinholters · 2016-04-20T08:27:52Z

AppVeyor failure looks unrelated. Any ideas?

tkelman · 2016-04-20T08:50:03Z

DictChannel not defined. Have seen it before, unrelated but annoying.

amitmurthy · 2016-04-20T09:09:37Z

@tkelman , can you post a link / gist of the DictChannel error?

tkelman · 2016-04-20T10:06:29Z

appveyor doesn't overwrite past logs, check the history for the previous build

edit: https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.684/job/pptqpykoho0d71h2

timholy · 2016-04-20T10:27:22Z

I imagine that you tried simply @deprecate_binding CentralizedAbs2Fun centralizedabs2fun, and something was broken? What was it?

martinholters · 2016-04-20T11:22:55Z

Just deprecating CentralizedAbs2Fun as centralizedabs2fun makes CentralizedAbs2Fun(m) work, but DataArrays also uses ::CentralizedAbs2Fun to dispatch on. Therefore, CentralizedAbs2Fun needs to be a type. Considering that DataArrays doesn't work on master anyway, maybe we shouldn't worry about this corner case too much and either @deprecate_binding CentralizedAbs2Fun centralizedabs2fun or remove it instead of deprecating it.

Nevertheless, being told to use Base.#* (instead of typeof(*)) is not exactly helpful, too.

nalimilan · 2016-04-20T12:49:05Z

Just deprecating CentralizedAbs2Fun as centralizedabs2fun makes CentralizedAbs2Fun(m) work, but DataArrays also uses ::CentralizedAbs2Fun to dispatch on. Therefore, CentralizedAbs2Fun needs to be a type. Considering that DataArrays doesn't work on master anyway, maybe we shouldn't worry about this corner case too much and either @deprecate_binding CentralizedAbs2Fun centralizedabs2fun or remove it instead of deprecating it.

If there's a way to avoid breaking this use case, please choose it. Even the current obscure deprecation message is better than a plain failure.

timholy · 2016-04-20T13:37:04Z

I don't think there's a way to do that. Wouldn't the strategy here break if m were a Float64?

martinholters · 2016-04-21T06:31:30Z

@nalimilan I wouldn't mind if it just was obscure. But it's actually really bad advise: "use Base.##557#558 instead." No, please don't! If you manage to interpret that as Base.(symbol("##557#558")), you get something that works ... until the next minor change in Julia changes this auto-generated symbol.

@timholy

julia> typeof(Base.centralizedabs2fun(0))
Base.##557#558{Int64}

julia> typeof(Base.centralizedabs2fun(0.0))
Base.##557#558{Float64}

julia> typeof(Base.centralizedabs2fun(0)).name.primary
Base.##557#558{#s281}

julia> typeof(Base.centralizedabs2fun(0.0)) <: typeof(Base.centralizedabs2fun(0)).name.primary
true

Now, this is obscure.

The following functors are deprecated: IdFun, AbsFun, Abs2Fun, ExpFun, LogFun, and ConjFun. Specialization for BitArray map! is done by dispatching on function type instead of using helper type BitFunctorUnary. Also a add a few tests for those.

Deprecate AndFun, OrFun, XorFun, AddFun, DotAddFun, SubFun, DotSubFun, MulFun, DotMulFun, RDivFun, DotRDivFun, LDivFun, IDivFun, DotIDivFun, ModFun, RemFun, DotRemFun, PowFun, MaxFun, MinFun, LessFun, MoreFun, DotLSFun, and DotRSFun. Rewrite specialization of BitArray map! to use function types and a helper type BitChunkFunctor for operations without a named function.

nalimilan · 2016-04-21T10:05:34Z

@martinholters I don't think anybody smart enough to find that Base.(symbol("##557#558")) works would be crazy enough to actually use it. :-) People will notice the warning is buggy. Anyway, my point is that a breaking change is the worst solution.

martinholters · 2016-04-21T11:12:46Z

@nalimilan Good point. Having found CentralizedAbs2Fun probably means you know what you're doing anyway.

There is another range of functors which are currently defined outside of functors.jl which I have removed rather than deprecated. I could also deprecated these. For ElementwiseMaxFun, ElementwiseMinFun, ComplexFun, DotFun it's easy as they can be replaced by typeof(f) for suitable f, so no harm in additing these to the deprecation list.

For EqX, NotEqZero, TrilFunc, TriuFunc, DroptolFunc, DropzerosFunc, DroptolFuncVec, DropzerosFuncVec it's more difficult because I have replaced their uses with anonymous inline functions. I could

Make only the instantiation work(EqX(5) is ok, f::EqX is not), which is relatively easy.
Go to same trouble as for CentralizedAbs2Fun which involves introducing named functions and replacing the anonymous ones used so far (to actually allow hooking in using dispatch).

I'd strongly favor 1.

martinholters · 2016-04-21T11:19:33Z

Let me add that I found no uses of all of these in the wild outside of Julia, with the exception of ComplexFun and DotFun used in SparseVectors.

…e.jl

martinholters · 2016-04-21T11:39:32Z

Rebased and added the missing "easy" deprecations.

timholy · 2016-04-21T15:16:54Z

As a deprecation strategy, my feeling is that this (i.e., this PR combined with JuliaLang/Compat.jl#184) is probably as good as it gets---in this case, it's a particularly complex problem for which there is no perfect solution. This should catch the large majority of cases. Moreover, since functors were not an exported interface, all uses outside of base knew they were reaching into an internal implementation. There's a long tradition of not caring too much about breakages that occur from such changes.

I also really like how you took the occasion of this big cleanup to notice a few loose ends and add missing tests, etc.

I'll give folks a few more hours to raise any remaining concerns, but I'll merge this soon if no one else does.

nalimilan · 2016-04-22T13:05:00Z

Nice cleanup!

JeffBezanson · 2016-04-22T14:16:40Z

@martinholters Thank you for tackling this! Well done.

martinholters · 2016-04-24T09:41:29Z

Thank you all for all the positive feedback I got! Not only is Julia a nice language to program in, it also seems to be programmed by nice people.

martinholters · 2016-04-25T07:00:19Z

Someone could close #15692 now.

ivarne · 2016-04-25T07:41:29Z

PS: @martinholters Github will automatically close related issues if you include something like closes #15692 or fixes #15692 (or similar) in the commit message.

tkelman reviewed Apr 8, 2016
View reviewed changes

martinholters force-pushed the depfunctors branch from 1e9829d to 32e9196 Compare April 11, 2016 07:07

KristofferC mentioned this pull request Apr 13, 2016

add fkeep! and children for sparse vectors #15585

Merged

martinholters force-pushed the depfunctors branch from 32e9196 to c9a8b36 Compare April 14, 2016 07:21

KristofferC mentioned this pull request Apr 14, 2016

Remove use of functors for sparse matrices and vectors #15696

Closed

KristofferC reviewed Apr 14, 2016
View reviewed changes

martinholters mentioned this pull request Apr 18, 2016

Add @functorize JuliaLang/Compat.jl#184

Merged

martinholters force-pushed the depfunctors branch from c9a8b36 to 5d3a2b5 Compare April 20, 2016 06:46

martinholters reviewed Apr 20, 2016
View reviewed changes

martinholters added 4 commits April 21, 2016 10:39

Move identity to operators.jl

1d0fa8a

Deprecate unary functors

0a72958

The following functors are deprecated: IdFun, AbsFun, Abs2Fun, ExpFun, LogFun, and ConjFun. Specialization for BitArray map! is done by dispatching on function type instead of using helper type BitFunctorUnary. Also a add a few tests for those.

Remove (now empty) test/functors.jl

e657621

martinholters added 5 commits April 21, 2016 13:22

Remove/deprecate special purpose functors and move Predicate to reduc…

7943016

…e.jl

Deprecate Func

1207e64

Move promote_op for Bool^Int to bool.jl

d8259d5

Remove (now empty) base/functors.jl

323a8d2

Remove functor references from docs

0429c38

martinholters force-pushed the depfunctors branch from 5d3a2b5 to 0429c38 Compare April 21, 2016 11:38

tkelman merged commit 21a7d4d into JuliaLang:master Apr 22, 2016

martinholters deleted the depfunctors branch April 22, 2016 11:14

nalimilan mentioned this pull request May 5, 2016

Fix functor-related deprecation warnings on Julia 0.5 JuliaStats/NullableArrays.jl#105

Merged


		function is_hermsym(A::SparseMatrixCSC, check::Func)
		function is_hermsym(A::SparseMatrixCSC, check::Function)

Defunctorize #15804

Defunctorize #15804

Conversation

martinholters commented Apr 8, 2016

andreasnoack commented Apr 8, 2016

KristofferC commented Apr 8, 2016

ViralBShah commented Apr 8, 2016

martinholters commented Apr 8, 2016

StefanKarpinski commented Apr 8, 2016

StefanKarpinski commented Apr 8, 2016

andreasnoack commented Apr 8, 2016

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

martinholters commented Apr 8, 2016

andreasnoack commented Apr 8, 2016

martinholters commented Apr 8, 2016

vtjnash commented Apr 13, 2016

martinholters commented Apr 13, 2016

tkelman commented Apr 13, 2016

martinholters commented Apr 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinholters commented Apr 14, 2016 • edited Loading

martinholters commented Apr 18, 2016

timholy commented Apr 20, 2016

martinholters commented Apr 20, 2016

Choose a reason for hiding this comment

martinholters commented Apr 20, 2016

tkelman commented Apr 20, 2016

amitmurthy commented Apr 20, 2016

tkelman commented Apr 20, 2016 • edited Loading

timholy commented Apr 20, 2016

martinholters commented Apr 20, 2016

nalimilan commented Apr 20, 2016

timholy commented Apr 20, 2016

martinholters commented Apr 21, 2016

nalimilan commented Apr 21, 2016

martinholters commented Apr 21, 2016

martinholters commented Apr 21, 2016

martinholters commented Apr 21, 2016

timholy commented Apr 21, 2016

nalimilan commented Apr 22, 2016

JeffBezanson commented Apr 22, 2016

martinholters commented Apr 24, 2016

martinholters commented Apr 25, 2016

ivarne commented Apr 25, 2016

martinholters commented Apr 14, 2016 •

edited

Loading

tkelman commented Apr 20, 2016 •

edited

Loading