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

map! only works on AbstractArray #38344

Open
simeonschaub opened this issue Nov 7, 2020 · 4 comments
Open

map! only works on AbstractArray #38344

simeonschaub opened this issue Nov 7, 2020 · 4 comments
Labels
domain:arrays [a, r, r, a, y, s] good first issue Indicates a good issue for first-time contributors to Julia status:help wanted Indicates that a maintainer wants help on an issue or pull request

Comments

@simeonschaub
Copy link
Member

Any reason for restricting the signature of map! to AbstractArray? I don't see why this shouldn't work:

julia> a, b = [1,2], (3,4)
([1, 2], (3, 4))

julia> map(*, a, b)
2-element Vector{Int64}:
 3
 8

julia> map!(*, a, a, b)
ERROR: MethodError: no method matching map!(::typeof(*), ::Vector{Int64}, ::Vector{Int64}, ::Tuple{Int64, Int64})
Closest candidates are:
  map!(::F, ::AbstractArray, ::AbstractArray) where F at abstractarray.jl:2259
  map!(::F, ::AbstractArray, ::AbstractArray, ::AbstractArray) where F at abstractarray.jl:2303
  map!(::F, ::AbstractArray, ::AbstractArray...) where F at abstractarray.jl:2350
  ...
Stacktrace:
 [1] top-level scope
   @ REPL[12]:1
@StefanKarpinski
Copy link
Sponsor Member

True: seems like only the output array needs to be restricted to AbstractArray.

@albheim
Copy link
Contributor

albheim commented Dec 15, 2020

Stumbled upon the same issue and wondered why map! was missing this functionality. This could maybe be something general that catches those cases (together with the multi argument ones mentioned in #23041).

function Base.map!(f, b::AbstractArray, a...)
    for (i,v) in enumerate(zip(a...))
        b[i] = f(v...)
    end
end

@kshyatt kshyatt added the domain:arrays [a, r, r, a, y, s] label Feb 6, 2021
@simeonschaub simeonschaub added status:help wanted Indicates that a maintainer wants help on an issue or pull request good first issue Indicates a good issue for first-time contributors to Julia labels Oct 19, 2023
@bvdmitri
Copy link
Contributor

I wouldn't use enumerate, but rather zip with the eachindex since there is no guarantee that the indexing of the output array is 1-based.

@albheim
Copy link
Contributor

albheim commented Oct 19, 2023

The current N-source implementation for map! requires the sources to be indexable, so it should work for tuples but not for generators.

Extending the type of the sources to be Union{Tuple, AbstractArray} initially seemed to work, though when tuples are involved the speed seems to go down a little.
What I later realised is that when the tuple was first julia crashed. It seems LinearIndices on a tuple does not create indices for the tuple, but assumes the tuple is the desired dimensions. This did not play well with @inbounds (as expected) and lead to the crash.

Doing a very simple version that captures remaining cases, and only requires the sources to be iterable is a lot slower.

function map!(f::F, dest::AbstractArray, As...) where {F}
    isempty(As) && throw(ArgumentError(
        """map! requires at least one "source" argument"""))
    for (i,v) in zip(eachindex(dest), zip(As...))
        dest[i] = f(v...)
    end
end

I don't know if it is preferable to just to have something simple to catch remaining cases, and then add more specialized methods for specific types when needed, or if is it preferable to skip it to avoid a slow path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] good first issue Indicates a good issue for first-time contributors to Julia status:help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

No branches or pull requests

5 participants