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

Group-By functionality for Scatter Widget #587

Closed
wants to merge 1 commit into from

Conversation

aak65
Copy link

@aak65 aak65 commented Mar 12, 2015

This is an improved grouping function for the Scatter plot that works faster and gets less confused. I thought it may be beneficial to have this as a separate pull from the time series stuff.

@ChrisBeaumont
Copy link
Member

2 quick comments:

  • You have an emacs-saved backup file (scatterwidget.ui~) in this PR, which you'll want to remove.
  • As I mentioned in Date-Time Support for Scatter Plot #565, I think it's best to implement this feature (which is really cool) as a custom scatterwidget subclass, probably stored with the plugins. Like what @astrofrog has been working on in Move all ginga widget code to a self-contained plugin #585. We'd like to start emphasizing putting new functionality in plugins/custom viewers, and then pulling particular features into the "standard" viewers more slowly. It shouldn't be much work to adapt your PR to do this, let us know if you get stuck

@aak65
Copy link
Author

aak65 commented Mar 12, 2015

Sure thing; I'll definitely give it a shot!

@astrofrog
Copy link
Member

@aak65 - just for information, I've merged in the pull request #590 which improves how we deal with plug-in functionality, and the documentation here:

http://www.glueviz.org/en/latest/python_guide/customization.html

has been updated. To add this as a plugin in glue, you simply need to add a file in glue/plugins inside which you define a class that inherits from ScatterWidget and overloads it appropriately. Then in your config.py file you can do:

from glue.config import qt_client
qt_client.add(MyScatterWidget)

and it should appear in the list of options. We can then see whether to enable it as a default plug-in. Let me know if you have any questions or need any help!

(you will need to rebase to get the latest plugin stuff)

@aak65
Copy link
Author

aak65 commented Mar 20, 2015

@astrofrog Thanks for that information! It's very helpful and I think I can figure this out. I'll work on it and update the PR once it's working so you guys can see if it meshes well.

Been a bit caught up with finals, but I'll have something up soon.

@astrofrog
Copy link
Member

@aak65 - just to check, what is the status of this pull request?

@aak65
Copy link
Author

aak65 commented Aug 19, 2015

@astrofrog - Sorry for the radio silence. I just graduated, moved across the country, and started a new job (my first real job). I really need to devote my full attention to settling in at the moment. In all honesty I may not come back to glue for a little while.

@astrofrog
Copy link
Member

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/)

@astrofrog astrofrog closed this Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants