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

Allow any f for sum/minimum/maximum(f, v::AbstractSparseVector) #29884

Merged
merged 2 commits into from
Apr 10, 2021

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Nov 1, 2018

At the moment only f=abs/abs2 have specialized implementations of sum/minimum/maximum(f, v::AbstractSparseVector), the other fs are handled by the generic AbstractVector methods.
This PR extends sparse-specific methods to any f.

The commit was extracted from #28535 since it should be a non-breaking backportable change.

cc @mbauman

@simonbyrne
Copy link
Contributor

simonbyrne commented Nov 2, 2018

This assumes f(zero(eltype(x))) is pure, which may not be the case. As far as I know, we don't have a way to check if a particular function or method is pure, but we could add a trait for this?

@StefanKarpinski
Copy link
Member

I think that's a fairly reasonable assumption though. What kinds of types are you thinking about where that would fail? BigInt and BigFloat because they allocate?

@mbauman
Copy link
Member

mbauman commented Nov 2, 2018

Yeah, we make that assumption throughout the sparse library. It's not the zero part that's the problem, it's the user-passed f — which could depend on global state (e.g. f(x) = x + rand(0:1)).

@test @inferred(sum(t -> true, spzeros(Float64, 8))) === 8
@test @inferred(sum(t -> abs(t) + one(t), spzeros(Float64, 8))) === 8.0

@test_broken sum(t -> true, spzeros(Float64, 0)) === 0 # reducing over an empty collection
Copy link
Member

Choose a reason for hiding this comment

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

I know it's unrelated, but maybe throw an @test_broken sum(t -> true, zeros(Float64, 0)) === 0 right next to this line so that way when one gets fixed the author will find the other.

@simonbyrne
Copy link
Contributor

Yeah, we make that assumption throughout the sparse library. It's not the zero part that's the problem, it's the user-passed f — which could depend on global state (e.g. f(x) = x + rand(0:1)).

So we do...

julia> map(x -> rand(), sparse(zeros(5)))
5-element SparseVector{Float64,Int64} with 5 stored entries:
  [1]  =  0.658879
  [2]  =  0.658879
  [3]  =  0.658879
  [4]  =  0.658879
  [5]  =  0.658879

@simonbyrne
Copy link
Contributor

Could this be done more generally by hooking into the mapreduce machinery?

@ViralBShah ViralBShah added the sparse Sparse arrays label Aug 21, 2019
@ViralBShah
Copy link
Member

@alyst This is an old PR. Any chance you'd be able to update it? It would be great to get this in, and then get the mapreduce stuff later on.

alyst added 2 commits January 10, 2020 14:56
generalize sum/minimum/maximum(abs/abs2, v::AbstractSparseVector) to
arbitrary f
@alyst
Copy link
Contributor Author

alyst commented Jan 10, 2020

tester_win32 failure is unrelated (permission denied), while the SparseVector tests had passed.

@vtjnash
Copy link
Member

vtjnash commented Apr 9, 2021

@ViralBShah is this good to go?

@ViralBShah ViralBShah merged commit fc69c9a into JuliaLang:master Apr 10, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
…aLang#29884)

* sum/minimum/maximum(f, v::AbstractSparseVector)

generalize sum/minimum/maximum(abs/abs2, v::AbstractSparseVector) to
arbitrary f

* add broken sum(f, [])==0 tests for reference
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
…aLang#29884)

* sum/minimum/maximum(f, v::AbstractSparseVector)

generalize sum/minimum/maximum(abs/abs2, v::AbstractSparseVector) to
arbitrary f

* add broken sum(f, [])==0 tests for reference
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
…aLang#29884)

* sum/minimum/maximum(f, v::AbstractSparseVector)

generalize sum/minimum/maximum(abs/abs2, v::AbstractSparseVector) to
arbitrary f

* add broken sum(f, [])==0 tests for reference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants