Rename ghost module to overlap #3830

rmsare · 2018-07-31T05:06:28Z

This replaces ghost with an identical overlap module and makes the same changes to the docs. All of the old ghost tests or documentation are kept as is, and tests are duplicated for overlap.

Calling an old function warns and returns the result from the corresponding function in overlap. I went ahead and replaced ghost.ghost with overlap.overlap for consistency, as it seemed confusing to be able to call overlap.ghost(...).

There are probably better ways to deprecate the old ghost module, too, but couldn't find any examples of deprecation warnings used elsewhere in dask so I tried to be conservative. Any pointers or feedback would be much appreciated!

Addresses #3639.

Tests added / passed
Passes flake8 dask

mrocklin · 2018-07-31T15:17:55Z

This looks really nice to me. cc @jakirkham @jni

All of the old ghost tests or documentation are kept as is, and tests are duplicated for overlap.

It's encouraing seeing that the old test suite works cleanly. Question for the group, do we want to keep it around, or clean it up / remove it?

mrocklin · 2018-07-31T15:19:33Z

Thank you for putting this together @rmsare . I think that this new name will help a lot of people find this functionality.

jni · 2018-07-31T23:39:21Z

Nice! I agree that this is the least-jargony name.

mrocklin · 2018-08-01T14:15:47Z

If no one else has any comments then I suggest that we remove the old test suite and then merge this in. If we want to keep some tests I think that they should probably only test the warning behavior, and leave the correctness tests for test_overlap.py . I don't think it's essential that we test the warning behavior though.

jakirkham · 2018-08-01T14:19:56Z

What do you mean by old tests?

mrocklin · 2018-08-01T14:24:47Z

test_ghost.py and test_overlap.py are near-copies of each other

…

On Wed, Aug 1, 2018 at 7:20 AM, jakirkham ***@***.***> wrote: What do you mean by old tests? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3830 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszFz5nnvnOUVEWZyq9wQskAlm3JyLks5uMbkfgaJpZM4VnmuD> .

rmsare · 2018-08-01T15:16:37Z

I agree with removing the old tests, done.

Not sure what the accepted practice would be on testing the warnings, but my preference is to not write specific tests for that kind of behavior.

mrocklin · 2018-08-01T20:07:04Z

Merging this tomorrow if there are no further comments

jcrist

Apologies for the late review here. Overall this looks fine, requested a few additional cleanups to the warnings.

jcrist · 2018-08-02T14:34:28Z

dask/array/ghost.py


-    index = tuple(index)
+    warn('ghost module will be renamed to overlap, '


Request to change "will be" -> "has been". This should be done throughout.

Also request to be a bit more descriptive, since this may show up deeper in other functions. Perhaps:

warn("the dask.array.ghost module has been renamed to dask.array.overlap, ...")

jcrist · 2018-08-02T14:37:45Z

dask/array/ghost.py

-    index = tuple(index)
+    warn('ghost module will be renamed to overlap, '
+         'use overlap.fractional_slice.',
+         DeprecationWarning)


DeprecationWarnings are ignored by default on non-python 2, so users won't see this warning (which is unhelpful). In other parts of dask we have been using raw warnings prefixed with the string "DeprecationWarning:", which seems to work fairly well.

Good point, completely forgot those are ignored with default warnings. I've updated the warning type and message text.

jcrist · 2018-08-02T14:38:52Z

docs/source/array-overlap.rst

+Map a function across blocks
+----------------------------
+
+Ghosting goes hand-in-hand with mapping a function across blocks.  This


Ghosting -> Overlapping?

jakirkham · 2018-08-02T14:47:55Z

dask/array/ghost.py

-    shape2 = [d if axes.get(i, 0) else 1 for i, d in enumerate(shape)]
-    result = reshapelist(shape2, seq)
-    return result
+    return overlap.expand_key(k, dims, name, axes)


 def ghost_internal(x, axes):


Should we rename ghost_internal to overlap_internal?

NVM. Overlooked the warning below.

jakirkham · 2018-08-02T14:50:24Z

docs/source/array-overlap.rst

+
+Consider two neighboring blocks in a Dask array.
+
+.. image:: images/unghosted-neighbors.png


Kind of nitpicky, but should we rename the images as well?

Worth noting there are SVGs like this as well that may need the same renaming. SVGs contain a docname parameter internally (why?) that includes the name too, which would need renaming as well.

jakirkham · 2018-08-02T14:50:58Z

docs/source/array-overlap.rst

+
+.. image:: images/unghosted-neighbors.png
+   :width: 30%
+   :alt: un-overlapped neighbors


non-overlapping

jakirkham · 2018-08-02T14:51:14Z

docs/source/array-overlap.rst

+
+.. image:: images/ghosted-neighbors.png
+   :width: 30%
+   :alt: overlapped neighbors


overlapping

jakirkham · 2018-08-02T14:51:20Z

docs/source/array-overlap.rst

+
+.. image:: images/ghosted-blocks.png
+   :width: 40%
+   :alt: overlapped blocks


overlapping

jakirkham · 2018-08-02T14:53:19Z

Should we (re)move docs/source/array-ghost.rst?

jakirkham · 2018-08-02T14:55:29Z

Looks pretty good, @rmsare. Added a few comments above. Thanks for working on this.

mrocklin · 2018-08-02T14:56:00Z

Perhaps rename and add a redirect?

…

On Thu, Aug 2, 2018 at 10:55 AM, jakirkham ***@***.***> wrote: Should we (re)move docs/source/array-ghost.rst? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3830 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszDJjSYir50QZpt26a95ICtOTlacHks5uMxLqgaJpZM4VnmuD> .

rmsare · 2018-08-02T18:50:19Z

Thanks @jcrist and @jakirkham ! I've addressed most of your comments.

TODO later today: Edit the SVGs, and redirect on the array-ghost.rst page.

Unfortunately these changes are pushed under my work account username, hope that's alright.

mrocklin · 2018-08-03T13:21:31Z

We should add a redirect in docs/source/conf.py. Look for redirect_files.

mrocklin · 2018-08-03T13:21:54Z

@jakirkham thank you for the extensive review here. Are you ok with the revised changes?

jakirkham · 2018-08-03T18:49:46Z

Just the redirect that you mentioned and there were a couple "ghost" SVGs remaining.

rmsare · 2018-08-05T23:32:53Z

@mrocklin Thanks for pointing out the redirect list. I think these changes take care of the rest of the comments above. Sorry for the delay on these.

mrocklin · 2018-08-06T14:07:06Z

Anything left to do here? @jakirkham are you ok with these changes?

jakirkham · 2018-08-06T14:43:19Z

There still appears to be docs/source/array-ghost.rst with a full doc page referencing da.ghost. Was that going to be replaced by a redirect? 😕

rmsare · 2018-08-06T15:20:21Z

@jakirkham Oops, it is deleted now. array-ghost.html redirects to array-overlap.html in any case.

jakirkham

LGTM. Thanks @rmsare. 😄

jcrist · 2018-08-06T20:13:55Z

Thanks @rmsare! Merging.

shoyer · 2018-08-08T00:55:49Z

This is causing xarray's test suite to fail (see pydata/xarray#2349).

In particular, please restore the top level import of ghost in dask/array/__init__.py, so code like da.ghost.ghost(...) gets a warning instead of failing with AttributeError.

mrocklin · 2018-08-08T01:03:48Z

@rmsare would you be willing to follow up here?

@shoyer thanks for the report

jcrist · 2018-08-08T01:26:31Z

Apologies @shoyer, this is fixed in #3861.

rmsare · 2018-08-08T12:54:26Z

@jcrist beat me to it. Thanks, and sorry to break the imports!

…

On Tue, Aug 7, 2018, 19:26 Jim Crist ***@***.***> wrote: Apologies @shoyer <https://github.com/shoyer>, this is fixed in #3861 <#3861>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3830 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADcJRMBDcwkXS10oRC0inzyY8Yy2c-vdks5uOj5LgaJpZM4VnmuD> .

rmsare added 12 commits July 30, 2018 22:00

Replace ghost by overlap in module and change function names

db879e3

Update overlap imports in array core and init

6c8cb33

Update references to overlap in array documentation

2a7ebf2

Replace ghost with deprecated module

f283552

Fix typo in add_dummy_padding warning

a6aae42

Add in deleted docstring for ghost.fractional_slice

f15fe83

Update ghost tests to use overlap names

63e8865

Fix value argument to overlap.constant

b5d4ed6

Add getitem import to pass tests

2115236

Remove ghost term from docs heading

0b28f28

Move getitem import to test_ghost

c0df877

Fix under-indenting due to renamed functions

b07a75e

Remove old ghost tests

1ebc041

jcrist reviewed Aug 2, 2018

View reviewed changes

jakirkham reviewed Aug 2, 2018

View reviewed changes

docs/source/array-overlap.rst Outdated

.. image:: images/ghosted-neighbors.png

:width: 30%

:alt: overlapped neighbors

Copy link

Member

jakirkham Aug 2, 2018

Choose a reason for hiding this comment

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

overlapping

jakirkham reviewed Aug 2, 2018

View reviewed changes

docs/source/array-overlap.rst Outdated

.. image:: images/ghosted-blocks.png

:width: 40%

:alt: overlapped blocks

Copy link

Member

jakirkham Aug 2, 2018

Choose a reason for hiding this comment

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

overlapping

rmsare-lanl added 3 commits August 2, 2018 12:44

Use Warnings and clarify message text

6cf349c

Fix Ghosting -> Overlapping, and rename images in text

4e1d451

Rename PNG images

23f8dd2

Delete old PNG images

cf81143

rmsare added 4 commits August 5, 2018 17:01

Rename old SVGs

371ff62

Change SVG docname parameters

805b82f

Change 'overlapped' to 'overlapping' in SVGs

794022f

Add array-ghost.html -> array-overlap.html to redirects

d85769b

rmsare added 2 commits August 5, 2018 17:53

Update docnames with 'overlapping'

b8950ac

Fix .svg extension in docname

7d88a91

Delete array-ghost.rst

9764441

jakirkham approved these changes Aug 6, 2018

View reviewed changes

jcrist merged commit 07cb89f into dask:master Aug 6, 2018

rmsare deleted the rename-ghost branch August 6, 2018 21:15

fujiisoup mentioned this pull request Aug 6, 2018

dask.ghost -> dask.overlap pydata/xarray#2349

Merged

shoyer mentioned this pull request Aug 8, 2018

Failing test for python=3.6 dask-dev pydata/xarray#2352

Closed

jcrist mentioned this pull request Aug 8, 2018

Re-add the ghost import to da __init__ #3861

Merged


		index = tuple(index)
		warn('ghost module will be renamed to overlap, '


		Consider two neighboring blocks in a Dask array.

		.. image:: images/unghosted-neighbors.png

Rename ghost module to overlap #3830

Rename ghost module to overlap #3830

Conversation

rmsare commented Jul 31, 2018

mrocklin commented Jul 31, 2018

mrocklin commented Jul 31, 2018

jni commented Jul 31, 2018

mrocklin commented Aug 1, 2018

jakirkham commented Aug 1, 2018

mrocklin commented Aug 1, 2018 via email

rmsare commented Aug 1, 2018

mrocklin commented Aug 1, 2018

jcrist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Aug 2, 2018 • edited

jakirkham commented Aug 2, 2018

mrocklin commented Aug 2, 2018 via email

rmsare commented Aug 2, 2018 • edited

mrocklin commented Aug 3, 2018

mrocklin commented Aug 3, 2018

jakirkham commented Aug 3, 2018

rmsare commented Aug 5, 2018 • edited

mrocklin commented Aug 6, 2018

jakirkham commented Aug 6, 2018

rmsare commented Aug 6, 2018

jakirkham left a comment

Choose a reason for hiding this comment

jcrist commented Aug 6, 2018

shoyer commented Aug 8, 2018

mrocklin commented Aug 8, 2018

jcrist commented Aug 8, 2018

rmsare commented Aug 8, 2018 via email

jakirkham commented Aug 2, 2018 •

edited

rmsare commented Aug 2, 2018 •

edited

rmsare commented Aug 5, 2018 •

edited