10BC0 Removed normalization of arrows in 3D quiver by DanHickstein · Pull Request #5458 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Removed normalization of arrows in 3D quiver #5458

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Nov 16, 2015
Merged

Removed normalization of arrows in 3D quiver #5458

merged 10 commits into from
Nov 16, 2015

Conversation

DanHickstein
Copy link
Contributor

I removed the normalization of the arrows in 3D quiver to match the behavior of the 2D quiver plot.

Also, I changed the default pivot point to be ‘tail’ in order to match the 2D quiver plot.

Finally, clarified the comment about normalizing the ‘uvw’ variable.

This was suggested here:

#4211

@DanHickstein
Removed normalization of arrows in 3D quiver
a204ae0 
I removed the normalization of the arrows in 3D quiver to match the
behavior of the 2D quiver plot. Also, I changed the default pivot point
to be ‘tail’ in order to match the 2D quiver plot. Also, clarified the
comment about normalizing the ‘uvw’ variable.
@WeatherGod WeatherGod added this to the next major release (2.0) milestone Nov 10, 2015
WeatherGod
WeatherGod reviewed Nov 10, 2015
View reviewed changes
lib/mpl_toolkits/mplot3d/axes3d.py Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhm, I think you mean to assign to a variable called "normalize"

@WeatherGod
Copy link
Member

This will also likely need to update the image tests, I think (or at least have some of them use normalize=True).

@DanHickstein
Copy link
Contributor Author

I fixed the typos. I don't know much about the tests, but I added "normalize=True" to the quiver3d_demo in order to keep it's behavior the same.

WeatherGod
WeatherGod reviewed Nov 10, 2015
View reviewed changes
examples/mplot3d/quiver3d_demo.py Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a space after the comma.

@WeatherGod
Copy link
Member

The tests are located in lib/mpl_toolkits/tests/test_mplot3d.py

@tacaswell
Copy link
Member

Sorry, I was confused about normalize vs the angle kwarg on quiver 🐑

Still think this is a good idea, but not for the reason I used to.

This needs an entry in https://github.com/matplotlib/matplotlib/tree/master/doc/api/api_changes with a note about how to return to the original behavior.

tacaswell
tacaswell reviewed Nov 13, 2015
View reviewed changes
doc/api/api_changes/2015-11-12-DDH.rst Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, sorry to be picky

@tacaswell
Copy link
Member

If you don't normalize the vectors how does length come in?

@tacaswell
Copy link
Member

In general 👍 .

@DanHickstein
Improved api change description
5bcf302 
Fixed typo, changed quotes, improved descriptions.
@DanHickstein
Copy link
Contributor Author

Thanks!
As far as I can tell, the length argument just scales the arrows. It's just a single number (nor an array), so it just scales them all together.

@DanHickstein
Copy link
Contributor Author

I slightly modified the description of the length keyword argument to indicate that it doesn't actually set the length of the arrows when normalize is False (it just scales them).

Let me know if there are other suggestions for this PR.

@tacaswell
Copy link
Member

👍 from me. Leave this to @WeatherGod to push the button.

WeatherGod added a commit that referenced this pull request Nov 16, 2015
@WeatherGod
Merge pull request #5458 from DanHickstein/quiver-fixes
174f9d8 
Removed normalization of arrows in 3D quiver
@WeatherGod WeatherGod merged commit 174f9d8 into matplotlib:master Nov 16, 2015
@WeatherGod
Copy link
Member

Thank you @DanHickstein! @tacaswell, I take it that this will need to be cherry-picked to v2.0.x branch?

@tacaswell
Copy link
Member

Yes

WeatherGod added a commit that referenced this pull request Nov 16, 2015
@WeatherGod
Merge pull request #5458 from DanHickstein/quiver-fixes
bf6e976 
Removed normalization of arrows in 3D quiver
@WeatherGod
Copy link
Member

cherrypicked to v2.0.x as bf6e976

@DanHickstein
Copy link
Contributor Author

Awesome! Thanks for your help!

@tacaswell
Copy link
Member

@DanHickstein Thank you for your work!

@DanHickstein DanHickstein deleted the quiver-fixes branch November 16, 2015 15:57
@WeatherGod WeatherGod mentioned this pull request Dec 1, 2015
Closed
@Zhuoliulz
Copy link

Feedback: Awsome! this modified file works properly and solves the "quiver" problems perfectly! Love your work!

@QuLogic QuLogic mentioned this pull request Dec 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
API: default changes topic: mplot3d
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@DanHickstein @WeatherGod @tacaswell @Zhuoliulz
0