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

Axes3D quiver: variable length arrows #4211

Closed
khs26 opened this issue Mar 11, 2015 · 22 comments
Closed

Axes3D quiver: variable length arrows #4211

khs26 opened this issue Mar 11, 2015 · 22 comments

Comments

@khs26
Copy link

khs26 commented Mar 11, 2015

Would there be interest in allowing the length argument to Axes3d.quiver() optionally be an array instead of just a single float?

It's a feature I'd like, but I wasn't sure whether to do it properly and make a PR or if there's a good reason for keeping things as is.

@tacaswell
Copy link
Member

You can also specify the (u, v, w) components. I think specifying length to be a vector as well would be degenerate and cause confusion.

@khs26
Copy link
Author

khs26 commented Mar 12, 2015

Sure, but in that case, shouldn't the normalisation of the u, v, w components be disabled?

I haven't really use masked numpy arrays, but I think the snippet below from mplot3d.py forces the u, v, w components to always represent a unit vector:

        # Normalize rows of UVW
        # Note: with numpy 1.9+, could use np.linalg.norm(UVW, axis=1)
        norm = np.sqrt(np.sum(UVW**2, axis=1))

        # If any row of UVW is all zeros, don't make a quiver for it
        mask = norm > 1e-10
        XYZ = XYZ[mask]
        UVW = UVW[mask] / norm[mask].reshape((-1, 1))

@khs26
Copy link
Author

khs26 commented Mar 12, 2015

Just to confirm, if I turn off the normalisation, things do seem to work as I'd hoped. I don't know whether there would be undesired knock-on effects from this change though.

@tacaswell
Copy link
Member

That does seem like what it should be doing (as that is how 2D quiver works).

@tacaswell
Copy link
Member

Can you put in a PR making that change?

@khs26
Copy link
Author

khs26 commented Mar 12, 2015

Sure. I'll make one and reference this issue. I'm working on something else at the moment, but should get round to it in a few hours.

@tacaswell
Copy link
Member

Thanks!

Don't stress too much about getting it done right away the deadline for this to make it into the next version is July (you also, don't procrastinate too long).

@khs26
Copy link
Author

khs26 commented Mar 13, 2015

Another small thing, where should the arrows be?

At the moment, the arrows seem to point from x-u, y-v, z-w to x, y, z. This also seems counterintuitive. I guess there are three sensible options:

  1. Leave it as is.
  2. Have the arrows point from x, y, z to x+u, y+v, z+w so that the arrows start at the specified x, y, z coordinates.
  3. Centre the arrows about x, y, z, so they point from x-u/2, y-v/2, z-w/2 to x+u/2, y+v/2, z+w/2.

My feeling is that option 3 is the most intuitive of the three. However, a quick look at StackOverflow suggests that the 2d quiver does option 2 and consistency is probably what we're going for.

@khs26
Copy link
Author

khs26 commented Mar 13, 2015

Here is an image demonstrating the current state of things. It's a representation of a molecule, so the green lines are bonds between atoms on the x, y, z coordinates made using axes3d.plot. The arrows are from axes3d.quiver.

Illustrative example prior to fixing normalisation

@WeatherGod
Copy link
Member

The documentation states that the xyz coordinates are the default locations
for the tip of the arrows. You can use the "pivot" keyword argument to
change that. That feature might only be in the master branch though.

On Fri, Mar 13, 2015 at 9:25 AM, Kyle Sutherland-Cash <
notifications@github.com> wrote:

Here is an image demonstrating the current state of things. It's a
representation of a molecule, so the green lines are bonds between atoms on
the x, y, z coordinates made using axes3d.plot. The arrows are from
axes3d.quiver.

[image: Illustrative example prior to fixing normalisation]
https://camo.githubusercontent.com/28a5f82ead15e54da2fb32527c6894a9f852c817/687474703a2f2f692e737461636b2e696d6775722e636f6d2f755a594a762e706e67


Reply to this email directly or view it on GitHub
#4211 (comment)
.

@tacaswell tacaswell added this to the next point release milestone Mar 13, 2015
@khs26
Copy link
Author

khs26 commented Mar 13, 2015

The pivot keyword for the 2d quiver plots seems to default to tail. Shouldn't they ideally be consistent? I would have thought that having the API call be essentially the same for 2d and 3d quiver, just with two extra sets of coordinates.

@tacaswell
Copy link
Member

Yes, they should be consistent.

Despite my normal insistence on maintaining the API, I think this is an ok
default to change as the 3D quiver is new and it should match the 2D
defaults.

On Fri, Mar 13, 2015 at 1:30 PM Kyle Sutherland-Cash <
notifications@github.com> wrote:

The pivot keyword for the 2d quiver plots seems to default to tail.
Shouldn't they ideally be consistent? I would have thought that having the
API call be essentially the same for 2d and 3d quiver, just with two extra
sets of coordinates.


Reply to this email directly or view it on GitHub
#4211 (comment)
.

@tacaswell tacaswell modified the milestones: next point release, proposed next point release Jul 16, 2015
@DanHickstein
Copy link
Contributor

I agree that the normalization should be removed (or maybe made into a keyword argument defaulting to False?). Also, pivoting from the tail seems like the most sensible to match 2D quiver. It seems like this never got changed...

Should I put in a PR for this?

@WeatherGod
Copy link
Member

The sooner, the better, as the 2.0 release is getting put together right
now. 2.0 will allow for changes in default settings.

On Tue, Nov 10, 2015 at 11:18 AM, Danhickstein notifications@github.com
wrote:

I agree that the normalization should be removed (or maybe made into a
keyword argument defaulting to False?). Also, pivoting from the tail seems
like the most sensible to match 2D quiver. It seems like this never got
changed...

Should I put in a PR for this?


Reply to this email directly or view it on GitHub
#4211 (comment)
.

@DanHickstein
Copy link
Contributor

Okay, PR created. Hopefully I did it correctly. I'm not too GitHub savvy. :)

@mdboom
Copy link
Member

mdboom commented Nov 10, 2015

@WeatherGod: As we've been making default changes, for many we've been adding a method to get the old behavior (usually just through matplotlib.style.use('classic')). How important do you think it is to do that for this as well? I think for things of fairly low usage, it's a little more ok to "just break" and not have a safety valve to get the old behavior.

@DanHickstein
Copy link
Contributor

It would be easy enough to include an option for the normalization. Though it's hard for me to think of a scenario where someone would desire this behavior.

@tacaswell
Copy link
Member

The use case is when you are only interested in the direction, not the
magnitude. I can imagine a case where the size variation is big enough that
you can not see both the biggest and smallest vectors.

On Tue, Nov 10, 2015, 13:50 Danhickstein notifications@github.com wrote:

It would be easy enough to include an option for the normalization. Though
it's hard for me to think of a scenario where someone would desire this
behavior.


Reply to this email directly or view it on GitHub
#4211 (comment)
.

@DanHickstein
Copy link
Contributor

Yes, I can see that there are some circumstances where the user would want to normalize their data, but should this be built into the quiver function? It's not in the 2D case...

@WeatherGod
Copy link
Member

@mdboom, +1 on having an option to normalize since the feature already
exists, +0 on not bothering to add an rcParam so that the "classic" style
would pick up the normalization behavior.

On Tue, Nov 10, 2015 at 2:05 PM, Danhickstein notifications@github.com
wrote:

Yes, I can see that there are some circumstances where the user would want
to normalize their data, but should this be built into the quiver function?
It's not in the 2D case...


Reply to this email directly or view it on GitHub
#4211 (comment)
.

@DanHickstein
Copy link
Contributor

Sounds good. I updated the PR to include a "normalize" keyword argument that defaults to False and controls the behavior.

@WeatherGod
Copy link
Member

Closed by #5458

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants