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

reverse(Iterators.filter) can be incorrect if predicate is impure #50440

Closed
adienes opened this issue Jul 6, 2023 · 2 comments
Closed

reverse(Iterators.filter) can be incorrect if predicate is impure #50440

adienes opened this issue Jul 6, 2023 · 2 comments
Labels
domain:iteration Involves iteration or the iteration protocol

Comments

@adienes
Copy link
Contributor

adienes commented Jul 6, 2023

From docstring of reverse

reverse(itr) is an iterator over the
same collection but in the reverse order.

yet consider

julia> mutable struct RollingMax
           m
       end

julia> function (rm::RollingMax)(x)
           if x > rm.m
               rm.m = x
               return true
           else
               return false
           end
       end

julia> arr = rand(10);

julia> Iterators.filter(RollingMax(0), arr) |> collect
5-element Vector{Float64}:
 0.20543016348395104
 0.40777744342276
 0.6118207748963927
 0.6765659450947831
 0.8719846507332757

julia> Iterators.filter(RollingMax(0), arr) |> Iterators.reverse |> collect
2-element Vector{Float64}:
 0.839252025474182
 0.8719846507332757

personally I think reverse(f::Filter) should just be removed; it cannot be implemented correctly without knowing the purity of the predicate. another option is to add a warning in the docs? or maybe a third option is to somehow dispatch reverse only when flt can be proven pure---not sure how this is technically possible though.

@adienes
Copy link
Contributor Author

adienes commented Aug 1, 2023

could this be labeled with bug and iteration ?

@brenhinkeller brenhinkeller added the domain:iteration Involves iteration or the iteration protocol label Aug 4, 2023
@adienes
Copy link
Contributor Author

adienes commented Aug 7, 2023

closing as #50497 is now merged

@adienes adienes closed this as completed Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:iteration Involves iteration or the iteration protocol
Projects
None yet
Development

No branches or pull requests

2 participants