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

Suggestion: kick mapslices out of abstractarray.jl #23645

Closed
StephenVavasis opened this issue Sep 8, 2017 · 3 comments
Closed

Suggestion: kick mapslices out of abstractarray.jl #23645

StephenVavasis opened this issue Sep 8, 2017 · 3 comments
Labels
arrays [a, r, r, a, y, s]

Comments

@StephenVavasis
Copy link
Contributor

The function mapslices defined in abstractarray.jl is really complicated. In fact, it is so complicated that I wouldn't be able to maintain it, and I suspect that there are only a few core developers who would be able to. The reason it is so complicated is that it tries to anticipate various use-cases that will arise in subtypes of AbstractArray, which I think is a practice generally discouraged in languages with type hierarchies.

Besides its inherent un-maintainability, another reason to drop it is that the definition of mapslices as a fallback makes certain sparse matrix methods silently fall back to dense matrix methods, but not others. For example, mapslices(sum,speye(1_000_000),1) is fast but median(speye(1_000_000),1) takes forever, as does mapslices(sum,speye(1_000_000),2). Of course, a fast median routine for sparse matrices does not currently exist in the language and would have to be written, but I think that forcing the user to write it (or to explicitly convert his/her sparse matrix to a dense matrix) is preferable to falling back to a slow method, especially since the fall-back is inconsistent.

So I propose that mapslices be dropped from abstractarray.jl and instead rewritten separately for a few subtypes.

@timholy
Copy link
Member

timholy commented Sep 9, 2017

@bramtayl has a very nicely written mapslices equivalent in JuliennedArrays. (It does some other cool stuff, too.) When I get a few moments to breathe I'm hoping to help migrate it to Base.

But we can consider the sparse case when we do. Use of view might help (you only need to create a specialized implementation of median(::SubArray{SparseMatrixCSC}) then), but I am concerned about the implications of side effects in the function. Like I said, I haven't had time to check in to @bramtayl's latest thoughts, there may be good solutions already.

@bramtayl
Copy link
Contributor

bramtayl commented Sep 9, 2017

No haven't thought about spare arrays at all. Definitely an important thing to consider during any future overhaul.

@KristofferC
Copy link
Member

I guess not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

5 participants