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

Implement by-value iterator for owned arrays #986

Merged
merged 7 commits into from
Apr 22, 2021
Merged

Implement by-value iterator for owned arrays #986

merged 7 commits into from
Apr 22, 2021

Conversation

bluss
Copy link
Member

@bluss bluss commented Apr 22, 2021

Implement IntoIterator for Array. This uses the same code for "dropping unreachable elements" that .move_into() does (from the #932 PR).

New features:

  • Array::into_iter (same for ArcArray, CowArray)

Fixes #196

These return views (don't edit the array in place), so must_use is
appropriate here.
Layout is fully pub (but hidden) for legacy reasons - used from
NdProducer trait. It's not a stable feature.
@bluss bluss force-pushed the intoiterator branch 2 times, most recently from 929f6a8 to d5d0892 Compare April 22, 2021 19:12
@bluss bluss merged commit 9f868f7 into master Apr 22, 2021
@bluss bluss deleted the intoiterator branch April 22, 2021 21:10
@bluss bluss added this to the 0.15.2 milestone Apr 22, 2021
@nilgoyette
Copy link
Collaborator

Thank you @bluss . This was included in 0.15.2 and I was surprised to see that it broke our project. It's not a problem at all, it was super easy to fix! I'm more curious about which of the following is true:

  1. This was an API change so it shouldn't have been included in a patch version.
  2. I shouldn't have been using into_iter with ndarrays because it was iterating on references anyway. I should have been using iter.

Or none of them? Or both ^^

@bluss
Copy link
Member Author

bluss commented May 19, 2021

Do you have an example of the code?

@nilgoyette
Copy link
Collaborator

Here's one example

fft_zscores // Array1 or f32 or f64
    .into_iter()
    .enumerate()
    .filter(|(_, &v)| v.abs() > threshold)
    .map(|(idx, &v)| Outlier { ... }

I had to remove both &.

@bluss
Copy link
Member Author

bluss commented May 19, 2021

Thanks. Do you recall the blog post https://blog.rust-lang.org/2021/05/11/edition-2021.html and the part about IntoIterator for arrays, it might be interesting? As it says:

Usually we categorize this type of breakage (adding a trait implementation) 'minor' and acceptable.

So that means (2) which applies here too. I'm suprised but I could see that it can happen.

I would recommend that you convert this code by using .iter(), not using the by move iterator unless you need to IMO.

@nilgoyette
Copy link
Collaborator

Oh, ok, that anwers my question. Thank you.

I don't want to abuse of your time, but may I ask you why using .iter() instead of removing the & is a better idea? I fail to see why one method is better in general than the other.

@bluss
Copy link
Member Author

bluss commented May 19, 2021

you can see it if you read the implementation? :) It's not a coincidence that we reached version 0.15.x before this method was implemented, it was not so easy to do and it has to do some convoluted stuff (in some cases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IntoIterator on ArrayBase
2 participants