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

Fix #19561 (sparse map/broadcast where the output eltype is not a concrete subtype of Number) #19589

Merged
merged 1 commit into from
Jan 1, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Dec 14, 2016

This pull request makes sparse map and broadcast work not only where the output eltype is a concrete subtype of Number providing zero. Specifically, this pull request handles the following cases: (1) The output eltype is concrete and either (1a) a subtype of Number providing zero or (1b) not a subtype of Number but supports comparison with 0. (2) The output eltype is abstract, but each output value either is (2a) of a subtype of Number providing zero or (2b) not of a subtype of Number but supports comparison with 0. Fixes #19561. Best!

@kshyatt kshyatt added the domain:arrays:sparse Sparse arrays label Dec 14, 2016
@Sacha0 Sacha0 added kind:bugfix This change fixes an existing bug domain:broadcast Applying a function over a collection labels Dec 14, 2016
@tkelman
Copy link
Contributor

tkelman commented Dec 14, 2016

trying not to commit to a more public iszero yet?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 14, 2016

trying not to commit to a more public iszero yet?

Trying to fix this bug without sparking... extensive, peripherally related design discussion :).

@kshyatt
Copy link
Contributor

kshyatt commented Dec 18, 2016

Is this mergeable?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 18, 2016

Is this mergeable?

#17623 now introduces iszero functionality, so perhaps best to revise this pull request once #17623 merges (later this week). Thanks for the bump! :)

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 23, 2016

Will rebase on #17623 and #19690 if/when the latter clears. Best!

@Sacha0 Sacha0 added this to the 0.6.0 milestone Dec 25, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Dec 31, 2016

Rewritten with #17623 and #19690 in. (This pull request now performs zero-checking via a Base.SparseArrays.HigherOrderFns-internal function _iszero, which wraps Base.iszero from #17623 for Numbers and AbstractArrays but provides more permissive fallback behavior [such that this pull request still addresses the regression where the output eltype is e.g. Union{Float64,String}]). Best!

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 1, 2017

Having the @tkelman seal of approval, absent objections I plan to merge once AV clears. Best!

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 1, 2017

(AV x86_64 failure unrelated.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays:sparse Sparse arrays domain:broadcast Applying a function over a collection kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants