Skip to content

Added basic implementation of windows iterators #306

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Apr 6, 2017

Conversation

Robbepop
Copy link

@Robbepop Robbepop commented Apr 6, 2017

Features:

  • Windows struct based on BaseIter
  • free function fn windows used internally, similar to fn exact_chunks
  • added to ndarray trait fn windows(&self, ...) for a.windows(Dim(...)) usage
  • also include some tests that also test for edge cases
  • maybe some useful docs are still missing (no time, yet)

Addresses issue #276.

@bluss
Copy link
Member

bluss commented Apr 6, 2017

Thanks, it looks good.

  1. I guess the macro_use on si can be removed since s![] is not used anymore.
  2. Same problem I found in my chunk code, needs to check that the window's ndim is the same as the view's ndim. https://github.com/bluss/rust-ndarray/blob/master/src/iterators/chunks.rs#L127-L130 One can use ndassert, it's an assertion that is always enabled but has a long output with debug assertions.
  3. We want this to be an NdProducer too, I think it fits into the macro over in the new and better chunk code. I can fix that up after merge.
  4. Tests are failing due to the prerelease version of ndarray in master, don't worry.

Good tests so it looks good.

The whole ndproducer thing has really taken off since we started discussing this, so there hasn't been a clear goal to work towards, shifting all the time :D Now that we have an Indices ndproducer we know that we can in theory produce anything we want as an ndproducer (including the ragged edges chunks, if we need that).

@Robbepop
Copy link
Author

Robbepop commented Apr 6, 2017

Thanks for the fast feedback.

Should be fixed now. :)

Since I haven't that much time at the moment I'd be happy if you take over the implementation of the NdProducer thingy for the Windows iterator if you want.

The whole ndproducer thing has really taken off since we started discussing this, so there hasn't been a clear goal to work towards, shifting all the time :D Now that we have an Indices ndproducer we know that we can in theory produce anything we want as an ndproducer (including the ragged edges chunks, if we need that).

I think I haven't yet fully understood the NdProducer details but it sounds really cool! :)

@bluss
Copy link
Member

bluss commented Apr 6, 2017

Thanks! the ndarray tests themselves pass, so we can merge.

use IntoDimension;

pub struct Windows<'a, A: 'a, D> {
iter : ::iter::Iter<'a, A, D>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignment like this, it's a minor issue. I see what it's trying to do, but I prefer to keep this consistent and not do this. Such small things can always be fixed up afterwards, so I try to not focus on them. But it's good to mention, for the future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay good to know. I just made it out of habit as I try to simply stick to the project's conventions. :)
I wish one day we can automate this formatting.^^

@bluss bluss merged commit 1acb431 into rust-ndarray:master Apr 6, 2017
@bluss
Copy link
Member

bluss commented Apr 6, 2017

Quick plug: #rust-sci is a new irc channel we are trying to start and get going. One more rust channel!

@Robbepop
Copy link
Author

Robbepop commented Apr 6, 2017

I guess "sci" simply stands for "science"?
Do you think it will attract enough community members?

@bluss
Copy link
Member

bluss commented Apr 6, 2017

We wanted it to be everything about numeric and scientific stuff in Rust

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants