Add __getitem__ slicing

[bgrant]

Add support for slicing (including incomplete indexing) through a DistArray's __getitem__. Currently only BlockMaps and NoDistMaps are supported, and only slices with step-sizes of 1 are supported.

Slices return DistArray views as one would expect.

This required a small bit of refactoring

1. I added a couple small attributes to NoDistMaps from BlockMaps for convenience
2. I consolidated two separated implementations of sanitize_indices and expanded its functionality and usage.

Slicing currently throws errors when a resulting slice would have a dimension of size 0, but maybe Kurt's upcoming changes will resolve this.

[kwmsmith]

This is great -- will review soon. I recommend we merge the reduction stuff first, and get the 0-dimensional and n-dimensional with shape entry of 0 working before merging this.

I think slicing the other distribution types is possible with certain constraints in place -- we should discuss. Those should come in future PRs, of course.

Other features for future PRs:

• step size >1 and < 0,
• Slicing cyclic with block_size==1 and slice step==1
• Evaluate slicing block cyclic with slice step==1,
• Slicing unstructured -- I think we can do it with an extra roundtrip.

[bgrant]

Also for a future PR:

• __setitem__ slicing. I think this will be easier than __getitem__ was, since you don't have to return a new DistArray.

[kwmsmith]

To allow us to implement this on other map subclasses in the future, can we put a slice() method on the map subclasses, and have Distribution.slice() call its map slices in a polymorphic way?

[cowlicks]

The merges from master make this hard to review. Could you rebase in the future?

[bgrant]

@cowlicks: I'm not really sold on history rewriting, but I could be convinced. What about the merge commits make this hard to review?

[cowlicks]

What is wrong with rewriting history locally? No one has a copy of your local branch, so you could rebase before you push. So you wouldn't need to add a merge at the end of the PR. I agree that we should not rewrite the history of any branch on DistArray's public repo. I think personal dev forks are usually okay to rewrite, unless another dev has expressed interest in working off one of your forks.

Generally I review a PR by looking at the list of commit messages. Then trying to understand how they are accomplishing the goal of the PR. From there, I read each commit consecutively. And make sure it does what the commit message says. And that it does nothing weird or buggy.

However this does not work if you carefully craft all your commit then throw a merge in at the end. Since the merge changes a bunch of stuff potentially related to your PR. A bunch of my comments are probably obsolete, and everything after the commit it should probably be re-reviewed.

I realize there is the other reviewing strategy of click on "files changed" and read everything. This is generally what I have to do when I realize a bunch of commits overlap. However this basically leaves you reviewing a giant diff with no commit messages. Which is frustrating because you have no idea what the changes are intended to do except for what is in the PR message.

[kwmsmith]

@bgrant -- what about creating a slice_owners() method on the client-side maps that handles the slice case, and perhaps rename owners() to index_owners() (which handles the other case)? the if isinstance(..., Integral) and elif isinstance(..., slice) looks like a code smell to me that suggests breaking out a separate method.

Also, same question for the local side -- what about adding a local_from_global_slice() method to the local map subclasses?

[kwmsmith]

It seems like this error case should be dealt with immediately after the santitize_indices() call -- reason for handling it here?

[bgrant]

That seems correct.

[bgrant]

Okay @kwmsmith : comments addressed.

[coveralls]

Coverage increased (+0.38%) when pulling 911925b on feature/add-slicing into de18b91 on master.