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

Deprecate vectorized abs methods in favor of compact broadcast syntax #18558

Merged
merged 1 commit into from
Sep 21, 2016

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Sep 17, 2016

This PR deprecates all (?) remaining vectorized abs methods in favor of compact broadcast syntax. Ref. #16285, #17302, #18495, #18512, and #18513. Best!

@tkelman
Copy link
Contributor

tkelman commented Sep 17, 2016

is there an abs(A::AbstractArray) method somewhere?

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 17, 2016

Worth noting: Unlike with float, real, etc., the remaining vectorized abs methods never alias their inputs. This PR should be less controversial than #18495, #18512, and #18513 as a result.

is there an abs(A::AbstractArray) method somewhere?

The broader vectorized abs methods were macro generated and hence deprecated in #17302. Best!

@kshyatt
Copy link
Contributor

kshyatt commented Sep 21, 2016

@tkelman is this good to go?

@tkelman tkelman merged commit aebf3ac into JuliaLang:master Sep 21, 2016
@tkelman
Copy link
Contributor

tkelman commented Sep 21, 2016

Actually these are all susceptible to the issue in #18590 (comment), where fusing will change the semantics and there isn't a great way of asking specifically for the structure-preserving version of this operation. So I think aliasing isn't the only controversial part about this whole set of PR's.

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 21, 2016

Actually these are all susceptible to the issue in #18590 (comment), where fusing will change the semantics and there isn't a great way of asking specifically for the structure-preserving version of this operation. So I think aliasing isn't the only controversial part about this whole set of PR's.

Agreed. I should be able to dedicate some time to experimenting with more general structure-preserving broadcast methods for sparse and structured matrix types in the next few days. Perhaps revert this merge and put the rest of these PRs on hold till we've better explored such methods? Thanks!

@pabloferz
Copy link
Contributor

pabloferz commented Sep 21, 2016

I've been trying to find a way of dealing with that issue and the only solution I can think of, would require the new type system in place.

@tkelman
Copy link
Contributor

tkelman commented Sep 21, 2016

If you'd like to revert this, that might make sense. Though it would de-conflict your set of other related PR's, which you might want to leave on hold until we figure out a plan here.

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 25, 2016

I've been trying to find a way of dealing with that issue and the only solution I can think of, would require the new type system in place.

@pabloferz, I would love to hear your thoughts. Perhaps in #18590? Best!

@tkelman
Copy link
Contributor

tkelman commented Dec 14, 2016

This was the only one of these we merged before realizing the issue, right? To refresh my memory it was roughly that abs.(A) and abs.(identity.(A)) would return different types due to fusing? That's fixed now by your more general sparse broadcast implementation, correct?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 15, 2016

This was the only one of these we merged before realizing the issue, right?

IIRC, correct.

To refresh my memory it was roughly that abs.(A) and abs.(identity.(A)) would return different types due to fusing?

IIRC, roughly yes: The issue was that fusion largely defeats broadcast specializations for particular function/type combinations, causing fused broadcast operations to fall back to dense broadcast for e.g. sparse and structured objects. (A separate issue impacts the vectorized big, float, complex, real, and conj methods, discussed in #18495.)

That's fixed now by your more general sparse broadcast implementation, correct?

For combinations of sparse matrices, yes. I also have code handling combinations of sparse matrices and scalars, unfortunately impacted by this issue described on discourse (but perhaps good enough as a functionality stopgap). Not being certain how to proceed on that front (whether to wait for the issue in that thread to improve, or instead pursue the other approach I mention there), my present focus is extending the preceding work to sparse vectors. More information in this discourse post. Best!

@martinholters
Copy link
Member

Did I mention somewhere that the output type should not be derived from inference results? I think that would make the issue you are having for the mixed sparse/scalar go away, no?

@Sacha0 Sacha0 deleted the devecabs branch December 23, 2016 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants