Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add plot_interactive method for 3D maps #1543

Merged
merged 4 commits into from Jul 27, 2018
Merged

Add plot_interactive method for 3D maps #1543

merged 4 commits into from Jul 27, 2018

Conversation

@facero
Copy link
Contributor

@facero facero commented Jul 18, 2018

Some code started at the coding sprint now fully functional.
I had to change a bit of the 2D plotting part and duplicate some of
code. Maybe there is a more elegant way to do that but this is a first
attempt.

Some code started at the coding sprint now fully functional.
I had to change a bit of the 2D plotting part and duplicate some of
code. Maybe there is a more elegant way to do that but this is a first
attempt.
@facero
Copy link
Contributor Author

@facero facero commented Jul 18, 2018

You need to have the ipywidgets installed. They are usually installed by defaults by you might wanna check.
conda install -c conda-forge ipywidgets

Can people try this on their machine to see if it works.
A simple working example is shown below:

# This is a simple test to show how a 3D Map object is plotted
# This only works in a Jupyter notebook with inline plots

%matplotlib inline
from gammapy.maps import WcsNDMap

cubefile="$GAMMAPY_EXTRA/datasets/vela_region/gll_iem_v05_rev1_cutout.fits"
m = WcsNDMap.read(cubefile)
m.plot()

Outputs this :
capture d ecran 2018-07-18 a 17 31 47

@adonath adonath added the feature label Jul 18, 2018
@adonath adonath added this to the 0.9 milestone Jul 18, 2018
@adonath adonath self-assigned this Jul 18, 2018
Copy link
Member

@adonath adonath left a comment

Thanks @facero for working on this. I've quickly tested it and it seems to work well, even for large datasets such as the Fermi-LAT galactic diffuse model. I've left a few inline comments.

In addition I have a few general comments, we should discuss about:

  • Even if I like plotting the spectrum in addition, I think it does not generalize very well to other axes, such as time or event id. So I'm leaning towards removing it again.
  • With the interactive visualization the plot method is really large and some functionality, such as passing and returning a WCSAxes object is not possible anymore. So I tend to rather introduce a separate plot_interactive() method and keep the .plot() method unchanged. Ideally it can be even reused in .plot_interactive().
try:
from ipywidgets.widgets.interaction import interact, fixed
import ipywidgets as widgets
except:

This comment has been minimized.

@adonath

adonath Jul 18, 2018
Member

No need to catch the ImportError here, as the if the import fails it will raise an ImportError anyway.

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

ok

mapND=fixed(self)
)
def _plot_interactive(mapND, index, stretch='linear'):
"""

This comment has been minimized.

@adonath

adonath Jul 18, 2018
Member

For this hidden function no docstring is needed, as the user will never see it, just remove it.

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

ok

This comment has been minimized.

@joleroi

joleroi Jul 19, 2018
Contributor

I can be nice for developpers ...!

This comment has been minimized.

@adonath

adonath Jul 19, 2018
Member

I think in this case the arguments are self-explaining by their name and use and the docstring does not really add any additional information. I found it rather distracting, when reading the code, because it is very unusual to have large blocks of comments on the 2nd indentation level. But I agree e.g. when a hidden function can be still used stand alone or is a hidden method on a class... but I think we do not have general policy in Gammapy, I think some hidden methods have a docstring some do not.


fig = plt.gcf()

fig.set_size_inches(12, 5)

This comment has been minimized.

@adonath

adonath Jul 18, 2018
Member

Is it possible to remove this line, because it prevents the user from setting the figure size from outside. E.g. by:

plt.figure(figsize=(16, 8))
map_nd.plot()

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

I think this should be easy to fix.

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

So turns out that this is not an easy fix as for some reason the fig = plt.gcf() needs to be created inside the function. When I tried to pass it as an argument no plot is shown.

This comment has been minimized.

@adonath

adonath Jul 19, 2018
Member

The call to fig = plt.gcf() is OK, I just meant hard-coding the size of the figure in line 574, as this prevents the user from setting it from outside. E.g. I like to adapt the figure size, when plotting survey images to make them appear larger on the screen.

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

If I don't specify anything I get a ~square image like that:

image
When nothing is specified, giving this plt.figure(figsize=(16, 8))before the .plot() doesn't change the size ratio. So not sure how to handle this.

This comment has been minimized.

@adonath

adonath Jul 19, 2018
Member

Maybe you could try the same using the mpl rc context manager:

import matplotlib as mpl
with mpl.rc_context(rc={'figure.figsize': (16, 8)}):
    wcs_nd.plot()

If you think this is to complex, then maybe add an optional figsize argument to the plot_interactive() method?

description='Plot stretch'),
mapND=fixed(self)
)
def _plot_interactive(mapND, index, stretch='linear'):

This comment has been minimized.

@adonath

adonath Jul 18, 2018
Member

It would be great to still pass **kwargs to the ax1.imshow() command below.

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

I've been trying a few things and searching the internet and could not find a working example. the thing is that for each parameter in @interact you have to specify whether it will be fixed or interactive. I don't how @interact handles optional keywords.

This comment has been minimized.

@adonath

adonath Jul 19, 2018
Member

The kwargs collected in .plot() is just a plain python dict, that can be passed as a fixed parameter to the _plot_interactive() method without the ** operator, here is a minimal example:

def h(c):
    print(c)

def f(a, **kwargs):
    print(a)
    def _g(b, kwargs):
        print(b)
        h(**kwargs)   
    _g('arg b', kwargs)

This should work, no?

This comment has been minimized.

@facero

facero Jul 26, 2018
Author Contributor

yes I just implemented that and it worked thanks.

-------
#TODO: currently returns nothing.
Not sure if interact can return something.
Was producing errors when I tried.

This comment has been minimized.

@adonath

adonath Jul 18, 2018
Member

I think for every call to the callback function a new Axes object is created, so I guess it's not possible to return those.

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

agreed. When I tried it was producing weird errors

@@ -468,7 +468,10 @@ def downsample(self, factor, preserve_counts=True):

def plot(self, ax=None, fig=None, add_cbar=False, stretch='linear', **kwargs):

This comment has been minimized.

@adonath

adonath Jul 18, 2018
Member

If we only support interactive plotting of one map axes, I think we should add an axes argument to specify which axis should be plotted.

This comment has been minimized.

@facero

facero Jul 19, 2018
Author Contributor

For now the plot function only works for 3D Maps therefore there is no axes selection. I agree that we should implement that in the mid-term.

@cdeil cdeil added this to To do in gammapy.maps via automation Jul 18, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 18, 2018

@facero - Thanks!

Agree with all comments by @adonath .

Separate methods are better for the user (each a simpler API) and for us to maintain (e.g. review, test).

For the spectrum line plot: I would also tend to maybe not add it.

Often one wants it for a region, and that's harder to configure. I'm pretty sure that things like https://js9.si.edu/ or http://aladin.u-strasbg.fr/AladinLite/ or http://ginga.readthedocs.io/ or new astro visualisers will develop really nice Jupyter Lab plugins and become a great interactive environment. It will just take a year or two. So for now, I'd suggest to keep it really simple concerning code additions to Gammapy, and maybe put the line plot or other interactive things into a tutorial notebook. Note that there's no need to have those as methods attached to the class, one can always put them as standalone functions and take the map object as input.

That said, @facero - if you really want this feature in, if it's a separate plot method, the cost / complexity for us is really low, I'd also be OK to put it in and try it out for a while (possibly reviewing again whether to keep it in Gammapy before the v1.0 release in the fall).

@facero
Copy link
Contributor Author

@facero facero commented Jul 19, 2018

Thanks for the quick review. All comennts by axel are easy to implement. The only one for which I haven't found a solution is the handling of **kwargs with interact.

The two main points raised are whether to keep the 2nd plot and whether to put this function in .plot() or .plot_XXX().

  • .plot() vs .plot_XX() : I'm in favor of keeping one plot function. I understand that this complicates the routine a bit but from a user point of view, I just want to do .plot() on all my objects. Having a too large diversity of functions, we might end up in people not using them because they simply don't know that they exist. In addition, it will be strange to have .plot() that works only for 2D Maps and .plot_interactive() that works only for >3D arrays and not 2D arrays. People will get used to .smooth() and .plot() function and introducing several more will be confusing. @registerrier @AtreyeeS any opinion ?

  • For the 2nd plot, I have no strong opinion, regarding @adonath comment, this will work if axes is a time dimension and will show a light curve. The 2nd plot could be an option set to False by default.

@AtreyeeS
Copy link
Member

@AtreyeeS AtreyeeS commented Jul 19, 2018

This looks nice @facero !
I really like the adjoining line plot, so I will not vote for removing it completely.
We do need to think about how to handle this in case of hypercube maps though.

I have no strong opinion regarding point (1), ie, whether it should be one function or two separate functions. If the user is not in a jypyter notebook, but running a normal ipython session, trying to plot 3D maps will also give weird errors, no? Maybe then it would be better to have it as separate functions?

raise ImportError('ipywidgets is not installed.')

@interact(
index=widgets.IntSlider(min=0, max=self.data.shape[0] - 1, step=1, value=1,

This comment has been minimized.

@AtreyeeS

AtreyeeS Jul 19, 2018
Member

Can it be possible to have as many sliders as non-spatial axis?
And what will be plotted is passed to _plot_interactive is slice instead of index
where slice is the list of sliders. Then this will work for 4-d cubes as well, and the user can easily see if all the slices are proper...

This comment has been minimized.

@adonath

adonath Jul 19, 2018
Member

I also like this suggestion (as alternative to give the axis name in the plot method), but then the spectrum / profile plot has to be removed for sure (which is fine by me...).

Added the possibility to handle **kwargs
@facero
Copy link
Contributor Author

@facero facero commented Jul 26, 2018

I've modified the routine to separate the 2D and 3D parts. **kwargs can now be handled.
Minimal example is :

%matplotlib inline
from gammapy.maps import WcsNDMap

cubefile="$GAMMAPY_EXTRA/datasets/vela_region/gll_iem_v05_rev1_cutout.fits"
m = WcsNDMap.read(cubefile)

m.plot_interactive(cmap='gnuplot2')
@@ -528,6 +528,65 @@ def plot(self, ax=None, fig=None, add_cbar=False, stretch='linear', **kwargs):
ax.autoscale(enable=False)
return fig, ax, cbar


def plot_interactive(self, ax=None, fig=None, **kwargs):

This comment has been minimized.

@cdeil

cdeil Jul 26, 2018
Member

Please add at least a one line summary docstring.
Or even a full docstring with some short explanations what this is.

This comment has been minimized.

@facero

facero Jul 26, 2018
Author Contributor

Done.

facero and others added 2 commits Jul 26, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 27, 2018

I tried this out locally; initially the stretch widget didn't work for me, not sure why, now it does. I did some edits in bb13faf . Merging this now. If anyone has an issue or wants to improve further, please open a new issue or PR.

@facero - Thanks!

@cdeil cdeil merged commit c6175ac into gammapy:master Jul 27, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.maps automation moved this from To do to Done Jul 27, 2018
@cdeil cdeil changed the title First attempt for an interactive plotter of 3D Map Add plot_interactive method for 3D maps Aug 15, 2018
@facero facero deleted the facero:plot_3D_Map branch Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants