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.

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

Remove use of functors for sparse matrices and vectors #15696

Closed
wants to merge 2 commits into from

Conversation

KristofferC
Copy link
Member

One step towards #15692. cc @timholy

@KristofferC
Copy link
Member Author

Did some spot benchmarks:

triu! using #11685

# Master
julia> @time triu!(B);
  0.039143 seconds (4 allocations: 160 bytes)


# PR
julia> @time triu!(B);
  0.039338 seconds (5 allocations: 176 bytes)

sparse using #10694 (comment)

#Master
2.532434 seconds (27 allocations: 322.724 MB, 3.15% gc time)

#PR
2.347069 seconds (27 allocations: 322.724 MB, 3.80% gc time)

Enough for me...

for (op, fun, TF, mode) in [(:max, :MaxFun, :Real, 2),
(:min, :MinFun, :Real, 2),
(:complex, :ComplexFun, :Real, 1)]
for (op, fun, TF, mode) in [(:max, :max, :Real, 2),
Copy link
Member

Choose a reason for hiding this comment

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

You can remove one of the duplicate symbols here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, updated.

@KristofferC
Copy link
Member Author

I only ran the sparse tests locally but this seems to break something with a type promotion test for broadcasting in test/arrayops.jl

@KristofferC
Copy link
Member Author

Ok, reverting back to

(.*)(A::AbstractArray, B::AbstractArray) = broadcast_zpreserving(MulFun(), A, B)

should fix it. That is a pretty odd place to have a method definition for .* between all AbstractArrays...

@timholy
Copy link
Member

timholy commented Mar 30, 2016

If you're eager to eliminate this one too, you could presumably define a broadcast_zpreserving(f::Function, ...) and then have deprecated fallbacks like broadcast(::MulFun, ...) = broadcast_zpreserving(*, ...).

@KristofferC
Copy link
Member Author

Not sure I am that eager :P. Although, talking about deprecations.. does all of this need deprecations since some people might have written code that uses functors to call for example sparse?

@timholy
Copy link
Member

timholy commented Mar 30, 2016

If exported functions allow a functor input (I haven't checked), then presumably it needs a deprecation. Non-exported functions are at-your-own risk, so don't need one.

In terms of just deprecations, an easier path might be to deprecate just Base.MulFun() itself, and have it generate *. Then you don't need to write any deprecations for any functions that accept functor inputs. So one option is to let this PR sit until some nice person helps out with the overall task.

@KristofferC
Copy link
Member Author

AppVeyor timed out.

Sparse used to allow Union{Func, Function} as the combine function. Letting this sit is ok by me.


function is_hermsym(A::SparseMatrixCSC, check::Func)
function is_hermsym(A::SparseMatrixCSC, check::Function)
Copy link
Member

Choose a reason for hiding this comment

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

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

@KristofferC
Copy link
Member Author

Subsumed by #15804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants