Skip to content

change call scheme for any and all for AbstractArray #55671

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthias314
Copy link
Contributor

This PR makes three changes to the way any and all calls are transformed into 3-argument _any and _all calls for AbstractArray:

  • any(a) is transformed to any(identity, a) instead of calling 2-argument _any, and analogously for all. I think this has the following advantage: If somebody wants to implement any for a custom AbstractArray type, then it is now enough to define the 2-argument any. Currently also the 1-argument version has to be defined explicitly.

  • The restriction that the predicate f for the 2-argument versions of any and all be of type Function has been removed. None of the other methods for the 2-argument versions of any and all impose this, and I see no reason to have it here.

  • I've deleted the explicit definitions of the 2-argument versions of _any and _all. They are covered by the following line

    $(_fname)(A, dims; kw...) = $(_fname)(identity, A, dims; kw...)

    in the for loop a few lines later. (I believe that it doesn't matter anyway. As far as I can tell, the 2-argument _any and _all were only used for the 1-argument any and all and nowhere else.)

@mbauman
Copy link
Member

mbauman commented Sep 3, 2024

This change looks like a great idea!

I don't fully understand (yet) why any and all are defined differently than the above loop... if there is a reason to it at all. It seems like they could — and should — all be defined in exactly the same manner. And then it'd be nice to effect this change for all the reduction functions by re-jiggering the loop above in the same way you did here.

@mbauman mbauman added the fold sum, maximum, reduce, foldl, etc. label Sep 3, 2024
@matthias314
Copy link
Contributor Author

Do you want to move forward with this? If yes, do you want changes for other reduction function to be part of the same PR or separate? (I would prefer the second option.)

@jishnub
Copy link
Member

jishnub commented Sep 17, 2024

It's probably better to do these separately

@nsajko nsajko added the arrays [a, r, r, a, y, s] label Oct 4, 2024
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] fold sum, maximum, reduce, foldl, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants