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

Generalize VectorFieldPlot #701

Merged
merged 2 commits into from Jun 14, 2016
Merged

Generalize VectorFieldPlot #701

merged 2 commits into from Jun 14, 2016

Conversation

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Jun 1, 2016

This PR makes the VectorField element and plot mirror other elements more closely in that color and size can be mapped to any arbitrary dimension. It is a backwards compatibility breaking change but makes it more consistent and more powerful. I've used it extensively in my thesis for plots where the angle, magnitude and color are three separate dimensions:

image

@jbednar
Copy link
Member

@jbednar jbednar commented Jun 1, 2016

Clearly a very useful change, and improves consistency. It looks like parameter names (and meanings) have changed, which is fine -- at least then people will get a syntax error if they try to use the old one. When you did something similar to the Points object, generalizing how it treats extra dimensions, it caused a lot of silent and very confusing problems; are those likely to happen from this too? Vector fields are definitely less commonly used than Points, so it can't be as bad as that was, but it would be nice to know whether e.g. vectors will now suddenly start having crazy lengths and colors like Points did, for data that has more dimensions than were previously used.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jun 1, 2016

When you did something similar to the Points object, generalizing how it treats extra dimensions, it caused a lot of silent and very confusing problems; are those likely to happen from this too?

That was a change to the sizing behavior which did not change the API. In this case I should simply set the size_index to 3, which will match the previous behavior. I think I made a mistake in the Points case and enabled size scaling by default which probably should not be the case.

Edit: Also need to update docstrings.

@philippjfr philippjfr force-pushed the vectorfield_refactor branch from 8af0dbe to 6305b82 Jun 6, 2016
@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jun 6, 2016

Ready to merge.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jun 13, 2016

Looks good and the pr build passed.

I would merge if it weren't for this:

It is a backwards compatibility breaking change but makes it more consistent and more powerful.

Should we try to do something for backwards compatibility before merging?

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jun 14, 2016

Should we try to do something for backwards compatibility before merging?

Now that I've set the size_index appropriately the default behavior hasn't actually changed. So I think I'm wrong about backwards incompatibility, it just makes it more flexible now.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jun 14, 2016

The push build won't pass because the PR is slightly behind I think. If you want I can update it but I think it's ready to merge regardless.

@jbednar
Copy link
Member

@jbednar jbednar commented Jun 14, 2016

Sounds good to me!

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jun 14, 2016

Ok, given that we think backwards compatibility is retained and this is more flexible, I will merge it now.

@jlstevens jlstevens merged commit 5f2739e into master Jun 14, 2016
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 69.967%
Details
@philippjfr
s3-reference-data-cache Tests passing no test data changes required.
Details
@jlstevens jlstevens deleted the vectorfield_refactor branch Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants