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 map!(f, array) #40632

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allow map!(f, array) #40632

wants to merge 4 commits into from

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented Apr 27, 2021

Edit -- implements #31677, see discussion there.

I didn't find an issue, but did find some tests of non-AbstractArray cases where this is allowed:

julia/test/dict.jl

Lines 1182 to 1183 in 3d08f82

testdict = Dict(:a=>1, :b=>2)
map!(v->v-1, values(testdict))

julia> filter(x -> x.nargs==3, collect(methods(map!)))  # on master
[1] map!(f, iter::Base.ValueIterator{<:Dict}) in Base at dict.jl:717
[2] map!(f, iter::Base.ValueIterator{<:WeakKeyDict}) in Base at weakkeydict.jl:140
[3] map!(f, iter::Base.ValueIterator) in Base at abstractdict.jl:571

@fredrikekre
Copy link
Member

xref #31677

@mbauman
Copy link
Sponsor Member

mbauman commented Apr 27, 2021

Here's the rub:

  • map!(f, A, B, C) is kinda like* A .= f.(B, C); map(f, B, C) is like f.(B, C).
  • map!(f, A, B) is like A .= f.(B). map(f, B) is like f.(B).
  • map!(f, A) would be ???. map(f) is like f.().

*ignoring differences between map and broadcast, of course

@mcabbott
Copy link
Contributor Author

Sorry I missed the earlier discussion, somehow. Agree it's a bit awkward.

Is the difference between map!'s behaviour of stopping when any collection is exhausted, and broadcast!'s demanding equality (and extending 1) relevant? Maybe not, broadcast!(()->99, zeros(3)) isn't extending anything, unlike broadcast!(_->99, zeros(3), [pi]).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 27, 2021

Extending from @mbauman...unless we change map(f), then the last row would be:

map(f) is like A -> map(f, A)
and
map!(f, A) may be B -> map!(f, A, B) (I assume, by extension)

@brenhinkeller
Copy link
Sponsor Contributor

FWIW map and broadcast seem different enough that the parallel structure argument doesn't really seem like a problem to me..

@LilithHafner LilithHafner added the status:triage This should be discussed on a triage call label Oct 27, 2023
@mbauman
Copy link
Sponsor Member

mbauman commented Nov 1, 2023

I'm honestly not sure what I was trying to say there, hah. I think my point was:

  • the number of evaluations of f in map(f, args...) is fully defined by the iteration space of args..., and thus if they're missing, it's just a single scalar evaluation.
  • the number of evaluations in map(f, arg1, args...) is exactly identical to map!(f, output, arg1, args...)
  • the number of evaluations in map(f) would necessarily be different from map!(f, output).

I'm not so sure that's a problem anymore. But we would have to change the documentation. Currently:

map(f, c...) -> collection

Transform collection c by applying f to each element.

map!(function, destination, collection...)

Like map, but stores the result in destination rather than a new collection.

@JeffBezanson
Copy link
Sponsor Member

We now have map!(f, ::ValueIterator) for dicts, so there is a bit of precedent for this.

@LilithHafner
Copy link
Member

Triage discussed this (#31677 (comment))

@LilithHafner LilithHafner removed the status:triage This should be discussed on a triage call label Nov 9, 2023
@adienes
Copy link
Contributor

adienes commented Feb 9, 2024

with #52631 merged, maybe this can merge too before feature freeze?

closes #31677

closes #12277, in that no further changes to behavior are planned

@LilithHafner LilithHafner added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 9, 2024
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 9, 2024

As a (minor) breaking change, I think #52631 needs to be in at least public release before we can merge this? (which does mean we can merge this on Tuesday, just not today)

@LilithHafner
Copy link
Member

IMO it would be possible to keep this even if we revert #52631, so no need to wait a release; but I'm not in a hurry here.

@LilithHafner LilithHafner removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 9, 2024
@LilithHafner LilithHafner added this to the 1.12 milestone Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants