Negative indexing in distarrays.

[cowlicks]

I allow distarrays to take negative indices by converting the negative indices to positive ones in __getitem__ and __setitem__. This seems like the easiest way but it is probably not the best.

I originally tried altering the Map classes to take negative indices but there were other checks happening that were throwing index errors. So that way seemed pretty invasive.

Also, look at the tests. Are the places I added tests the only cases that we are currently checking indexing directly?

[kwmsmith]

Restarting Travis.

[cowlicks]

I've added the suggested tests. And re factored __get/setitem__ to use a single _normalize_index method which does validity checking and coercion.

[kwmsmith]

Why don't we put _normalize_index() inside owning_ranks()?

[cowlicks]

Hmmm I still don't like this @kwmsmith _normalize_index does not need any distribution information. It is just doing sanity checking and guaranteeing the index passed is standardized

owning_ranks does not sound like the name of a method that would mutate an index. It also already returns a list of targets, so it would have to return (targets, normalized_index). And this change would propagate to owning_targets. Which iterates over the index with owning_ranks. Which would make it return a bunch of extraneous standardized indices.

Putting it in owning_targets is another option but would make us do this

targets, index = owning_targets(index)

Looking at that does not make what is happening to the index obvious.

[kwmsmith]

On Wed, Apr 23, 2014 at 4:48 PM, Blake Griffith notifications@github.comwrote:

Hmmm I still don't like this @kwmsmith https://github.com/kwmsmith
_normalize_index does not need any distribution information.

It doesn't need any distarray information, either -- it's just a function.

It is just doing sanity checking and guaranteeing the index passed is
standardized

owning_ranks does not sound like the name of a method that would mutate
an index.

There's no mutation going on with _normalize_index or elsewhere -- we're
leaving the input index alone and returning a new one.

I'm in favor of changing the name owning_ranks to something else anyway.
Not sure what at the moment...

I want owning_ranks to be able to take any valid index (including
negative indices) and do the right thing. Currently it can't take negative
indices. This is a bug. If it is able to handle negative indices, then
distarray.getitem / setitem don't have to have any negative index
logic in them at all.

It also already returns a list of targets, so it would have to return (targets,
normalized_index).

Why would it have to return the normalized indices? We can just pass the
original index object to the localarray's checked_getitem() exactly as
it is now. We do have to call _normalize_index on the local side, too,
but the local array is already calling _santitize_indices, so we're not
adding much at all here.

The index handling stuff has to be defensive -- we don't know how it's
being called, as the user can write code that indexes the localarrays
directly, in which case we have to handle negative indices on the local
arrays. So we might as well implement those checks now.

Putting the normalization code inside the distribution object ensures it is
handled at the right location. If we put the normalization code outside,
then every time we interact with a distribution object, it is our
responsibility to make sure that we aren't passing in negative indices,
which is error prone.

And this change would propagate to owning_targets.

Not if owning_ranks just calls the normalization code as-is -- then
owning_targets doesn't have to change at all.

Which iterates over the index with owning_ranks. Which would make it return

a bunch of extraneous standardized indices.

Putting it in owning_targets is another option but would make us do this

targets, index = owning_targets(index)

Looking at that does not make what is happening to the index obvious.

Kurt W. Smith, Ph.D. ksmith@enthought.com
Enthought, Inc. http://www.enthought.com | 512.536.1057

[cowlicks]

It also already returns a list of targets, so it would have to return (targets,
normalized_index).

Why would it have to return the normalized indices? We can just pass the
original index object to the localarray's checked_getitem()

Well owning_targets does not expect negative indices, so index must be "normalized/sanitized" before owning_targets is called. And we need to call owning_targets before checked_getitem so that we know what targets to call it on, to avoid extra communication.

I could change owning_targets to allow negative indices. But if it does not return the postivified index, I will have to duplicate the negative index logic again in checked_getitem.

[kwmsmith]

On Mon, Apr 28, 2014 at 10:11 AM, Blake Griffith
notifications@github.comwrote:

It also already returns a list of targets, so it would have to return
(targets,
normalized_index).

Why would it have to return the normalized indices? We can just pass the
original index object to the localarray's checked_getitem()

Well owning_targets does not expect negative indices,

Any distribution method that works with indices (this includes
owning_targets and owning_ranks) must do the right thing with negative
and out-of-bounds indices. These methods are part of the distribution's
public API, so they have to have the logic for handling negative indices,
and raising an IndexError if given a bad index. Since owning_targets
is implemented on top of owning_ranks, if the latter calls the
normalization function, then both methods will do the right thing.

so index must be "normalized/sanitized" before owning_targets is called.
And we need to call owning_targets before checked_getitem so that we know
what targets to call it on, to avoid extra communication.

I could change owning_targets to allow negative indices. But if it does
not return the postivified index, I will have to duplicate the negative
index logic again in checked_getitem.

No duplication necessary. We just put the normalization function in a
common location, and both the client side and local side distribution
classes call it at the right spot.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/338#issuecomment-41570026
.

Kurt W. Smith, Ph.D. ksmith@enthought.com
Enthought, Inc. http://www.enthought.com | 512.536.1057

[cowlicks]

Ah okay, I agree negative indexing should be available to Distribution objects. I'll put them in there.

[cowlicks]

@kwmsmith how does this look?

[cowlicks]

Hold off on this until PR #349 is in.

[kwmsmith]

Closes #102

[cowlicks]

Oops, map apparently returns a generator in python3.

[cowlicks]

I think this is now ready for review, but PR #349 should go in first.