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

Conversation

Projects
None yet
3 participants
@philippjfr
Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

philippjfr commented Jun 6, 2016

Ready to merge.

@jlstevens

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jbednar commented Jun 14, 2016

Sounds good to me!

@jlstevens

This comment has been minimized.

Member

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

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
s3-reference-data-cache Tests passing no test data changes required.
Details

@philippjfr philippjfr removed the in progress label Jun 14, 2016

@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