-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add in place version of findall
function
#54722
Conversation
See also #43737 - the lazy version might be able to be copied into a vector. |
So you mean something like |
What I mean is that the lazy version of julia> lazy_findall(p, it) = (k for (k, v) in pairs(it) if p(v));
julia> collect(lazy_findall(isodd, [8,3,6,1,3,7,8,9])) |> print # eager version
[2, 4, 5, 6, 8]
julia> copyto!(zeros(Int, 5), lazy_findall(isodd, [8,3,6,1,3,7,8,9])) |> print # in-place version
[2, 4, 5, 6, 8] Here, the eager version is simply |
Has this implementation the same performances of my case? Because in your example this involves Iterators, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure this by itself is a good idea. Overwriting the existing data seems very counterintuitive, especially since the original array is not resized & keeps unrelated data around.
n = count(f, A) | ||
I = @view(A[1:n]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why count first and then loop over the array again? That's double the work, if you move the @view
to after the loop, you can just take the view from begin:(begin+count)
. In the loop you can still safely write into A
, there's no need for I
up front.
cnt = 1 | ||
@inbounds for (i, a) in enumerate(A) | ||
if f(a) | ||
I[cnt] = i | ||
cnt += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1-based indexing won't work correctly for e.g. OffsetArray
s. Not all AbstractArray
s are 1-based.
Ok so I think that this implementation is not an optimal one. Let’s close it, and wait for the Lazy implementation discussed above. |
Triage does not like this for a few reasons:
|
Ok let’s close this PR |
Hello,
I added the in place version of the
findall
function. This is very useful when it is iteratively called inside a function, maybe with some preallocated cache, avoiding to allocate a lot of memory.