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

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 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 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
Copy link
Member Author

Ready to merge.

@jlstevens
Copy link
Contributor

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

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

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 commented Jun 14, 2016

Sounds good to me!

@jlstevens
Copy link
Contributor

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
@jlstevens jlstevens deleted the vectorfield_refactor branch July 12, 2016 22:56
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