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

Allow variable symbol sizes in ScatterPlot #101

Merged
merged 15 commits into from
Mar 11, 2013
Merged

Conversation

kjordahl
Copy link
Contributor

Move functionality from VariableSizeScatterPlot to parent ScatterPlot class. Also allows ColormappedScatterPlot to use variable marker sizes, closing #97.

scatterplot-merge2

@kjordahl
Copy link
Contributor Author

I have two remaining concerns before I think this is ready to be merged:

  1. Performance could be better, especially for the case of constant symbol size.
  2. There is still a bug in indexing in ColormappedScatterPlot. I think it is fixed in ScatterPlot now.

@kjordahl
Copy link
Contributor Author

kjordahl commented Mar 3, 2013

I've addressed my previous concerns, and IMO this is now ready to merge. Any other opinions?

@pberkes
Copy link
Contributor

pberkes commented Mar 8, 2013

If you can give me a couple of days, I'd like to update the docs.

Pietro Berkes added 5 commits March 10, 2013 11:57
Since it is not that special anymore, I simplified it and moved it to the
'basic' directory. It was also added to the demo application.
@pberkes
Copy link
Contributor

pberkes commented Mar 10, 2013

I made a few updates to examples and documentation, the rest looks great. Unless somebody objects, I'll merge it tomorrow.

@kjordahl do you see anything stopping us from lifting the color up to scatterplot? performance?

@kjordahl
Copy link
Contributor Author

@pberkes Thanks.

I still think everything should ultimately go into the base scatterplot. I am concerned about performance, but don't have good tests for it yet. More than that, it would be really nice if every attribute that could apply to each marker could be specified as an array (particularly marker). Doing that right would take a little more work, but I don't think it's unreasonable.

Meanwhile, I think we should merge this as a step forward (that also fixes a previous bug), and I'll make a new branch to start working on a unified ScatterPlot class.

@cfarrow
Copy link
Contributor

cfarrow commented Mar 11, 2013

On Sun, Mar 10, 2013 at 8:50 PM, Kelsey Jordahl notifications@github.comwrote:

@pberkes https://github.com/pberkes Thanks.

I still think everything should ultimately go into the base scatterplot. I
am concerned about performance, but don't have good tests for it yet. More
than that, it would be really nice if /every/ attribute that could apply to
each marker could be specified as an array (particularly marker). Doing
that right would take a little more work, but I don't think it's
unreasonable.

You might find the flyweight pattern userful:
http://en.wikipedia.org/wiki/Flyweight_pattern. It handles performance and
memory issues associated with the "most things are the same but some are
different" pattern quite well. (The write up in Design Patterns is much
better than the wikipedia page, IMO.)

Meanwhile, I think we should merge this as a step forward (that also fixes
a previous bug), and I'll make a new branch to start working on a unified
ScatterPlot class.


Reply to this email directly or view it on GitHubhttps://github.com//pull/101#issuecomment-14694332
.

The information contained in this message is Enthought confidential & not
to be disseminated to outside parties without explicit prior approval from
sender. This message is intended solely for the addressee(s), If you are
not the intended recipient, please contact the sender by return e-mail and
destroy all copies of the original message.

@pberkes
Copy link
Contributor

pberkes commented Mar 11, 2013

@kjordahl I agree about merging this PR, but was interested in your opinion since you worked on this class.

@cfarrow Thanks for the pointer!

pberkes pushed a commit that referenced this pull request Mar 11, 2013
Allow variable symbol sizes in ScatterPlot
@pberkes pberkes merged commit 870a732 into master Mar 11, 2013
@pberkes pberkes deleted the feature-scatterplot-merge branch March 11, 2013 08:13
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.

None yet

3 participants