Add more features and clarification to Animators #3504

Cadair · 2019-11-14T15:00:43Z

This is a direct follow on from #3407 it adds the following functionality / cleanups:

Use modest_image from Glue to improve rendering of images in ArrayAnimatorWCS by reading less of the array. (Especially useful with dask arrays or memmapped arrays).
Allow passing through extra slider functions in the ArrayAnimator constructor, this lets you provide extra sliders for controlling the colourbar limits for instance.
Make the specification of button labels optional, by default it will use the function names.
Wire in an existing keyword argument to BaseFuncAnimator which allows the specification of a function to setup the plot as keyword argument.
Modify the call signature of the button call back functions so that they get passed the animator object, so that they can easily do things that modify the plot.
Add optional slider labels to BaseFuncAnimator and set them by default in ArrayAnimatorWCS to the world axis names if present or the world axis physical types if not.

ghost · 2019-11-14T15:00:48Z

Thanks for the pull request @Cadair! Everything looks great!

pep8speaks · 2019-11-14T15:00:50Z

Hello @Cadair! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file sunpy/extern/modest_image.py:

Line 12:1: E402 module level import not at top of file
Line 13:1: E402 module level import not at top of file
Line 14:1: E402 module level import not at top of file
Line 15:1: E402 module level import not at top of file
Line 17:1: E402 module level import not at top of file

In the file sunpy/visualization/animator/base.py:

Line 463:101: E501 line too long (105 > 100 characters)

In the file sunpy/visualization/animator/tests/test_wcs.py:

Line 143:5: E306 expected 1 blank line before a nested definition, found 0

In the file sunpy/visualization/animator/wcs.py:

Line 99:9: E303 too many blank lines (2)
Line 99:101: E501 line too long (118 > 100 characters)
Line 107:5: E303 too many blank lines (2)
Line 111:101: E501 line too long (107 > 100 characters)

Comment last updated at 2019-11-21 18:12:40 UTC

DanRyanIrish · 2019-11-14T15:24:25Z

Sounds potentially powerful. Would have to see a demo to fully understand though.

nabobalis · 2019-11-16T13:01:26Z

Rebase conflicted for me (I suspect something is off with my repo) so I just merged in master.

nabobalis · 2019-11-16T15:34:14Z

Not sure how the figs failed when they passed on my machine?!

* Pass the animator object to the button callback function. * Make button labels default to the names of the functions if not specified.

Cadair · 2019-11-21T17:14:49Z

There appears to be a codecov issue with uploading coverage on the py38-online build, so that test is not representative.

dstansby

Looks good to me, left some suggestions that you can take or leave as you please.

dstansby · 2019-11-21T17:44:31Z

licenses/GLUE.rst

@@ -0,0 +1,19 @@
+Glue - multidimensional data exploration


Maybe worth a note here referencing the file it is needed for?

dstansby · 2019-11-21T17:46:10Z

sunpy/extern/modest_image.py

@@ -0,0 +1,351 @@
+"""
+Modification of Chris Beaumont's mpl-modest-image package to allow the use of


Has this gone upstream/if not is it worth going upstream?

That you would have to ask @astrofrog

dstansby · 2019-11-21T17:46:16Z

sunpy/extern/modest_image.py

+    are skipped.
+
+    The interface of ModestImage is the same as AxesImage. However, it
+    does not currently support setting the 'extent' property. There


Change this bit of docstring, since this implementation does allow extent?

I don't really want to change this file as it's in extern, I just want to be able to copy it out of Glue if there are updates there?

And the reason we aren't just importing it from glue is...? I imagine it saves us a dependency but also means we have to update manually. Or I am not understanding externs?

yeah, it means we don't have to depend on glue, which is a large dependency with a lot of qt and other things, really not worth it.

sunpy/visualization/animator/base.py

dstansby · 2019-11-21T17:52:18Z

tox.ini

@@ -1,5 +1,5 @@
 [tox]
-envlist = py{36,37,38}-{offline,online,astropydev,numpydev,astropy31},build_docs,figure,conda
+envlist = py{36,37,38}-{offline,online,astropydev,numpydev,astropy31},build_docs,figure,figure_astropydev,conda


Why this change?

We forgot to add the new env to tox.ini in #3407

Co-Authored-By: David Stansby <dstansby@gmail.com>

DanRyanIrish

So long as the questions I have raise no concerns, this looks fine to me.

DanRyanIrish · 2019-11-21T21:59:31Z

sunpy/extern/modest_image.py

+    are skipped.
+
+    The interface of ModestImage is the same as AxesImage. However, it
+    does not currently support setting the 'extent' property. There


And the reason we aren't just importing it from glue is...? I imagine it saves us a dependency but also means we have to update manually. Or I am not understanding externs?

DanRyanIrish · 2019-11-21T22:06:45Z

sunpy/visualization/animator/base.py

        base_kwargs = {
-            'slider_functions': [self.update_plot] * self.num_sliders,
-            'slider_ranges': [[0, dim] for dim in np.array(data.shape)[self.slider_axes]]
+            'slider_functions': ([self.update_plot] * self.num_sliders) + slider_functions,


So does this mean the slider functions specified by the user in the slider_ranges kwargs are only extra slider functions, e.g. for a colourbar? Data axes' sliders are still derived internally?

Same for slider_ranges

Yes, indeed it does.

DanRyanIrish · 2019-11-21T22:07:46Z

sunpy/visualization/animator/wcs.py

+        extra_slider_labels = []
+        if "slider_functions" in kwargs and "slider_labels" not in kwargs:
+            extra_slider_labels = [a.__name__ for a in kwargs['slider_functions']]
+


Too many empty lines.

Cadair mentioned this pull request Nov 14, 2019

Even more animator features Cadair/sunpy#44

Closed

Cadair added this to the 1.1 milestone Nov 14, 2019

Cadair added the visualization Affects the visualization submodule label Nov 14, 2019

Cadair requested a review from a team November 14, 2019 15:02

Cadair force-pushed the even_more_animator_features branch 3 times, most recently from b2f9e9a to 0b4cf63 Compare November 14, 2019 17:05

nabobalis force-pushed the even_more_animator_features branch from 58d261b to abf57f6 Compare November 16, 2019 15:23

Cadair force-pushed the even_more_animator_features branch from abf57f6 to c2cb415 Compare November 18, 2019 12:27

nabobalis approved these changes Nov 20, 2019

View reviewed changes

Cadair self-assigned this Nov 20, 2019

Cadair force-pushed the even_more_animator_features branch 2 times, most recently from be67da1 to 3d524f2 Compare November 21, 2019 14:53

Cadair and others added 6 commits November 21, 2019 15:46

Use modest image from Glue to increase performance of ArrayAnimatorWCS

0bd5503

Allow specification of extra sliders in ArrayAnimator

7976056

Improve button support.

9bff805

* Pass the animator object to the button callback function. * Make button labels default to the names of the functions if not specified.

Support str labels

36a190c

Add slider labels to BaseFuncAnimator

26aa0ba

figure hashes update

7108cfc

Cadair force-pushed the even_more_animator_features branch from c5378f3 to 7108cfc Compare November 21, 2019 15:48

dstansby approved these changes Nov 21, 2019

View reviewed changes

Apply suggestions from code review

110fa60

Co-Authored-By: David Stansby <dstansby@gmail.com>

Add intersphinx link to MouseEvent

113c44c

DanRyanIrish reviewed Nov 21, 2019

View reviewed changes

DanRyanIrish approved these changes Nov 21, 2019

View reviewed changes

Cadair merged commit b877147 into sunpy:master Nov 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more features and clarification to Animators #3504

Add more features and clarification to Animators #3504

Cadair commented Nov 14, 2019 •

edited

Loading

ghost commented Nov 14, 2019 •

edited by ghost

Loading

pep8speaks commented Nov 14, 2019 •

edited

Loading

DanRyanIrish commented Nov 14, 2019

nabobalis commented Nov 16, 2019

nabobalis commented Nov 16, 2019

Cadair commented Nov 21, 2019

dstansby left a comment

dstansby Nov 21, 2019

dstansby Nov 21, 2019

Cadair Nov 21, 2019

dstansby Nov 21, 2019

Cadair Nov 21, 2019

DanRyanIrish Nov 21, 2019

Cadair Nov 21, 2019

dstansby Nov 21, 2019

Cadair Nov 21, 2019

DanRyanIrish left a comment

DanRyanIrish Nov 21, 2019

DanRyanIrish Nov 21, 2019

Cadair Nov 21, 2019

DanRyanIrish Nov 21, 2019

		@@ -0,0 +1,351 @@
		"""
		Modification of Chris Beaumont's mpl-modest-image package to allow the use of

Add more features and clarification to Animators #3504

Add more features and clarification to Animators #3504

Conversation

Cadair commented Nov 14, 2019 • edited Loading

ghost commented Nov 14, 2019 • edited by ghost Loading

pep8speaks commented Nov 14, 2019 • edited Loading

Comment last updated at 2019-11-21 18:12:40 UTC

DanRyanIrish commented Nov 14, 2019

nabobalis commented Nov 16, 2019

nabobalis commented Nov 16, 2019

Cadair commented Nov 21, 2019

dstansby 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

DanRyanIrish 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

Cadair commented Nov 14, 2019 •

edited

Loading

ghost commented Nov 14, 2019 •

edited by ghost

Loading

pep8speaks commented Nov 14, 2019 •

edited

Loading