search

SciTools / biggus

:no_entry: [DEPRECATED] Virtual large arrays and lazy evaluation.
http://biggus.readthedocs.io/
GNU Lesser General Public License v3.0
54 stars 27 forks source link

Implemented the NewAxesArray for np.newaxis indexing. #127

Closed pelson closed 9 years ago

pelson commented 9 years ago

A clear path existed for me to implement np.newaxis support so I took it. The PR is pretty big, but the vast majority is testing.

Apologies for stepping on your toes @rhattersley. Hope that is :+1:.

Alternative for #116 (only without the np.array support, which would be nice to have).

Some work from #124 needs to be pulled into this, so I will rebase once that is merged.

rhattersley commented 9 years ago

This is bizarrely similar to the code I was writing yesterday - right down to the name of the class and it's parameters! Convergent evolution in action?

pelson commented 9 years ago

Convergent evolution in action?

Good stuff. Apparently it isn't always bad to step on each others toes in a sprint (according to Scrum master training yesterday)

pelson commented 9 years ago

Or perhaps it was subliminal? https://github.com/SciTools/biggus/pull/116#discussion_r26714001 :wink:

rhattersley commented 9 years ago

Or perhaps it was subliminal? #116 (comment) :wink:

I suspect a bit of both. :grinning:

pelson commented 9 years ago

P.S. This needs a rebase, so if I were putting review effort anywhere, I'd start with #124. If you're curious about this, browse away! :smile:

pelson commented 9 years ago

@rhattersley - now that #124 is merged, do you want me to rebase and fixup the remaining tests, or would you like me to iterate based on your comments so far?

rhattersley commented 9 years ago

do you want me to rebase and fixup the remaining tests, or would you like me to iterate based on your comments so far?

I'd prefer to postpone the rebase for now if that's not going to cause too many problems.

pelson commented 9 years ago

I'd prefer to postpone the rebase for now if that's not going to cause too many problems.

I'll hold fire then. Actions will be forthcoming in new commit(s).

pelson commented 9 years ago

I've followed through with the major action of using _getitem_full_keys. There are one or two comments that still need addressing, but how is that shaping up?

rhattersley commented 9 years ago

how is that shaping up?

It's fine - please continue, and feel free to re-base now.

pelson commented 9 years ago

I've rebased and handled the cases which related the the latest PR.

pelson commented 9 years ago

I'm pulling on a rather slippery thread at the minute. First off, the code doesn't yet support:

>>> import numpy as np
>>> a = np.arange(3)[None, ..., None]
>>> a[1:2].shape
(0, 3, 1)

But this is pulling on a wider issue that you can do the following with a newaxis:

>>> a[(0, 0), ...].shape
(2, 3, 1)

In order to implement this in the simplest possible way, I'm tempted to suggest a BroadcastedArray which would be created when indexing with: BroadcastedArray(array, broadcast={0: 2}) where the keyword represents a mapping of axis to length.

rhattersley commented 9 years ago

As discussed offline yesterday, I'm OK with just raising a NotImplementedError for now.

pelson commented 9 years ago

As discussed offline yesterday, I'm OK with just raising a NotImplementedError for now.

@rhattersley - I've now added the commit which does the minimum possible to give sensible fail mechanisms on indexing of NewAxesArray.

rhattersley commented 9 years ago

Thanks @pelson. :+1:

pelson commented 9 years ago

For the record, I fixed both issues in #133.