Scatter Plot Grouping Plugin

[aak65]

I gave the scatter grouping plugin a shot, but haven't been able to get it to work completely yet. I was hoping you guys may be able to point me in the right direction.

I put everything in a plugin folder, but some parts of it (such as the util.py and the ui file) may belong better somewhere else. The issue I couldn't figure out is how to get the custom ui file to load properly. The extra grouping combo box isn't being seen by QWidget (line 324 in scattergroupwidget.ui).

Also, I put this in my config file:

from glue.config import qt_client
from glue.plugins.scatter_group.qt_widget import ScatterGroupWidget
qt_client.add(ScatterGroupWidget)

ps. I'm putting this up as a separate PR because I worked off a fresh branch on this, and I didn't want to mess up the comments and history on the 'grouping' branch.

[astrofrog]

@aak65 - I'll take a look at this now. I think the ui issue might be solved by #599 (already in this branch, it's another issue)

[astrofrog]

Ok, so the main issue here seems to be that ScatterWidget.__init__ calls _connect, but your _connect requires the ui file to be read in already. By that point, the ScatterWidget has loaded its own ui file, so it doesn't work.

At the moment you are overloading all of the stuff in ScatterWidget.__init__ anyway, so one quick workaround is to replace the super call with:

        super(ScatterWidget, self).__init__(session, parent)

so that it will skip the code in ScatterWidget.__init__ itself.

Another thing is that the load_ui call should be given the full path to the file, so:

        self.ui = load_ui(os.path.join(os.path.dirname(__file__), 'scattergroupwidget.ui'))

Finally, you should only inherit from ScatterWidget, not DataViewer, since ScatterWidget already inherits from DataViewer.

Now obviously, changing the super call is not the proper solution, and a better solution would be that the the ui file is loaded in a method that you can overload, and you then won't need to re-define __init__. So you could create a def load_ui(self) method on ScatterWidget then overload just that from your subclass. Similarly, I think the only other thing that changes is the code to set up the client, so you could also make that its own method _setup_client (basically we should not have to overload __init__ directly.

Now for some reason even with the hacks above the widget to edit the properties is not showing in the left panel, so I can look into that later. But at least it will no longer crash.

@ChrisBeaumont - do you agree that the solution is to have ScatterWidget.__init__ call methods rather than do everything itself?

[astrofrog]

Ah, it's an easy fix for the options not appearing - the call to load_ui should include self.option_widget to tell it where the options should appear:

self.ui = load_ui(os.path.join(os.path.dirname(__file__), 'scattergroupwidget.ui'), self.option_widget)

Ok, so to get things to work in the short term:

• Change the super call to:

super(ScatterWidget, self).__init__(session, parent)

• Fix the self.ui = line as shown above
• Don't inherit from DataViewer

Once you have that working, you can then try and make it so you don't overload all of __init__, so in ScatterWidget, define two new methods, setup_ui and setup_client that you can then overload in your sub-class, and then no longer overload __init__ directly.

[aak65]

@astrofrog Thank you for that explanation! I removed the __init__ overloading in client & qt_widget, defined those methods you suggested, and the plugin seems to be working fine.

This functionality is awesome and I think we can definitely make use of it for a few 'bio-glue' visualization plugins.

[ChrisBeaumont]

@astrofrog this logger module is new to me -- is that something you added? Before I was calling logging.getLogger(__name__) to organize loggers into per-module hierarchies. Is that not needed anymore?

[astrofrog]

Yes, I changed it when setting up the lazy loading of modules - the issue I ran into is that I think we really only should have one logger for the whole application otherwise I think it's hard to set the logging level globally - or am I wrong?

In other packages, such as Astropy, we have a single logger but it's still able to display in what sub-pacakge the message occurred. Are there other reasons we might want individual loggers?

[ChrisBeaumont]

@ChrisBeaumont - do you agree that the solution is to have ScatterWidget.init call methods rather than do everything itself?

Sounds right to me @astrofrog

[ChrisBeaumont]

Likewise, you can call super() here and just add the callback to gatt

[ChrisBeaumont]

Very Cool!

I left a bunch of small comments about using super() to cut down on code duplication, but this is looking great. Since this is bundled as a plugin instead of the main scatter viewer, I'm fine with merging this once its cleaned up. Unless @astrofrog has ideas about whether to put this in this repo or the experimental plugin repo? (I vote that it goes here, until we have an easy GUI way to install plugins from GH within the app).

Should we add a piece of documentation somewhere, describing how to enable this?

[astrofrog]

@ChrisBeaumont - putting it in the core package plugins is perfectly fine, as it's pretty generic. It would indeed be good to have a section in the docs describing available plugins. I like the idea of having a GUI plugin selector in future!

[astrofrog]

@aak65 - thanks for cleaning this up!

In general, for this kind of plugin, I wonder whether there might be a way to avoid duplicating the ui file, and whether we could instead adjust the widget once it has been loaded using load_ui. I'd need to look into it more to see how we can support this cleanly, but maybe @ChrisBeaumont has some ideas in the mean time?

[ChrisBeaumont]

I wonder whether there might be a way to avoid duplicating the ui file,

Yeah so if you wanted to cut down on duplication in the ui file, the strategy would be to override the function that builds the widget, call super() to construct the standard scatterplot widget, and then add your extra UI on top of that. In this case I think it's only a few extra widgets, so you could do that by instantiating widgets in python directly. Or you could have a (very tiny!) ui file that just adds the extra combo box, call load_ui a second time to build that widget, and then insert it into the right spot.

We probably don't want to make the assumption that all plugins can be created in this way

[ChrisBeaumont]

Ooo I like that we're failing builds where coverage decreases now :) @aak65 if we're going to put this in the main glue repository, we should add some tests

[astrofrog]

I'm going to close this since the scatter widget code has now been completely re-written, but hopefully this would be simpler to implement with the new viewer code.

@aak65 - if you are interested in porting over your code to the new viewers, just email me or reach out to me on Slack (you can get an invite here: https://glueviz-slack-invite.herokuapp.com/)