Skip to content

Conversation

bgrant
Copy link
Contributor

@bgrant bgrant commented May 19, 2014

The client only needs to use a LocalArray's checked_getitem or checked_setitem methods when it doesn't know for sure which engine owns a particular index; otherwise it can use a LocalArray's raw __getitem__ or __setitem__. With the addition of client-side maps, a DistArray always knows where all of its indices are unless one of its dimensions is an UnstructuredMap. The changes in this PR reflect this.

To implement this, I add the has_precise_index property to client-side Distribution objects. This property is False only when an UnstructuredMap is present.

Additionally, I remove the checked_getitem and checked_setitem methods from the LocalArray class, which forces a user to use Distarray.global_index.checked_{g,s}etitem, where they still reside. Previously, LocalArrays had a __getitem__ that took local indices and a checked_getitem that took global indices. This was confusing. Another option, if we wanted to be stricter with the Law of Demeter, would be to add these methods back but rename them something like global_checked_getitem.

bgrant added 9 commits May 19, 2014 14:18
See inline comment for a full description.
This way I don't need to add it separately to all the constructors.  There's also the small benefit of resilience to changing out the underlying maps.
A copy of them had already been added to LocalArray's GlobalIndex object, I just had to use it.
@bgrant bgrant added this to the 0.3 milestone May 19, 2014
Copy link
Contributor

Choose a reason for hiding this comment

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

Why all caps? I never took you for a FORTRAN 77 aficionado, @bgrant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Mon, May 19, 2014 at 5:18 PM, Kurt Smith notifications@github.comwrote:

Why all caps? I never took you for a FORTRAN 77 aficionado, @bgranthttps://github.com/bgrant
.

What? You didn't see my Project Euler solutions in FORTRAN 77?

https://github.com/bgrant/project-euler/blob/master/fortran/euler.f

But seriously, you're probably right. It seemed kind of like a constant or
a flag to me, but I suppose the fact that it's a function belies that.
Maybe I could rename it to allows_precise_indexing or
has_precise_indexing?

Copy link
Contributor

Choose a reason for hiding this comment

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

After all, to follow F77, you'd have to fit the name into 6 characters like PRCIDG or something totally cryptic like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Mon, May 19, 2014 at 5:28 PM, Robert David Grant <
notifications@github.com> wrote:

In distarray/dist/maps.py:

@@ -592,3 +592,12 @@ def get_dim_data_per_rank(self):
coord_and_dd = [zip(*cdd) for cdd in cart_dds]
rank_and_dd = sorted((self.rank_from_coords[c], dd) for (c, dd) in coord_and_dd)
return [dd for (_, dd) in rank_and_dd]
+

On Mon, May 19, 2014 at 5:18 PM, Kurt Smith notifications@github.comwrote:
Why all caps? I never took you for a FORTRAN 77 aficionado, @bgranthttps://github.com/bgrant
https://github.com/bgrant .
What? You didn't seem my Project Euler solutions in FORTRAN 77?
https://github.com/bgrant/project-euler/blob/master/fortran/euler.f

Nice! You should capitalize EVERYTHING, though. And what's with
explicitly declaring your function variables? Real programmers use
implicit typing.

But seriously, you're probably right. It seemed kind of like a constant
or a flag to me, but I suppose the fact that it's a function belies that.
Maybe I could rename it to allows_precise_indexing or
has_precise_indexing?


Reply to this email directly or view it on GitHubhttps://github.com//pull/377/files#r12819899
.

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

@bgrant
Copy link
Contributor Author

bgrant commented May 19, 2014

Updated @kwmsmith.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.28%) when pulling e52f102 on refactor/restrict-use-of-checked-getitem into 6bdecab on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.28%) when pulling 6201ff4 on refactor/restrict-use-of-checked-getitem into 6bdecab on master.

@kwmsmith
Copy link
Contributor

@bgrant looks good -- ignore my comment about the targets stuff earlier. Using owning_targets() is correct.

kwmsmith added a commit that referenced this pull request May 21, 2014
…ed-getitem

Restrict use of checked_{g,s}etitem
@kwmsmith kwmsmith merged commit 1ed353f into master May 21, 2014
@kwmsmith kwmsmith deleted the refactor/restrict-use-of-checked-getitem branch May 21, 2014 04:43
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.

4 participants