fix quiver3d incorrect arrow colors #27754

AlecVercruysse · 2024-02-07T20:29:34Z

PR summary

Closes #11759, which identifies that when setting individual arrow colors of a quiver3d plot, the arrow heads are colored incorrectly (differently than the bodies). PR #13742 attempted to address this issue but is now stale.

The default cycling logic in LineCollection should be good enough to handle the logic of assigning the correct property to each line (body, head 1, head 2) in an arrow. I switched the order that the lines were passed to the Line3DCollection to make use of this cycling. I also removed the logic that got rid of length 0 lines, since that messed up cycling (e.g., in the example below, only 9 lines are plotted but 10 colors are provided).

import numpy as np
import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import Axes3D

x = np.zeros(10)
y = np.zeros(10)
z = np.arange(10.)
dx = np.zeros(10)
dy = np.arange(10.)
dz = np.zeros(10)

fig = plt.figure()
ax = fig.add_subplot(111, projection='3d')

arrow_color = plt.cm.Reds(dy/dy.max())
ax.set_xlabel('x')
ax.set_ylabel('y')
ax.quiver(x, y, z, dx, dy, dz, colors=arrow_color)
ax.set_ylim(0, 10)
plt.show()

I filled out the below checklist with the idea that no changes need to be made to examples or documentation, as this is a bugfix. Running pytest test_quiver.py in matplotlib/lib/matplotlib/tests passes.

Thank you for reviewing!

PR checklist

"closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

AlecVercruysse · 2024-02-07T20:34:06Z

As an aside, at first I tried to run pytest "in the root directory of my development repository" (e.g. matplotlib/) as advised in the docs. That resulted in the below error:

E   ImportError: cannot import name '_version' from partially initialized module 'lib.matplotlib' (most likely due to a circular import) (/home/avercruysse/repos/matplotlib/lib/matplotlib/__init__.py)

I later got the test working by running pytest in lib/matplotlib/tests/. Am I running the tests correctly? Should that doc be updated to say that pytest needs to be run in lib/matplotlib/tests? Thanks!

AlecVercruysse · 2024-02-07T21:18:47Z

Whoops, looks like I was running the wrong tests. Now when I run pytest in lib/mpl_toolkits/mplot3d/tests, I get some interesting failures:

test_quiver3d image comparison fails with RMS of 0.002, but I can't find a difference:
result:

expected:

diff:
test_errorbar3d_errorevery fails similarly (RMS 0.003):
result:

expected:

diff:

Is it fair to replace the expected images with the result images?

oscargus · 2024-02-07T22:53:31Z

It is possible to set a tol argument to the image_comparison decorator. This is the slightly preferred method since the tolerance is indeed very small. If you can add a comment that says something like "remove tolerance when regenerating the test image" it would be appreciated (so that we know that it is not, e.g. architecture dependent, but an outcome of a minor numerical change).

- (avoid unnecessary computation otherwise)

AlecVercruysse · 2024-02-08T01:36:53Z

Thank you for the input! I've edited the test case accordingly.

AlecVercruysse · 2024-03-01T07:08:24Z

Hi, bumping this for review. I think this is a pretty minor change that makes a big difference, since as of now, attempting to color-code quiver3d arrows doesn't work without a convoluted work-around that requires people to understand the internals of quiver3d. Judging by Stack Overflow, it's something a lot of people (including me) run into. I've pasted a before and after image below in case that's necessary.

before:

after:

I think the fix as presented in this PR would cover 99% of all use cases (i.e., the length of the color list being the same length as the number of quiver arrows being plotted). Only in some edgecase where the length of the list of colors is not an even divisor of the length of the list of arrows would the old behavior revert, where arrowheads have a different color than their bodies.

If this edge-case needs to be covered, an alternative solution would be to construct a single line for each arrow (shaft and each head included), as I tried on another branch. This requires drawing the same line twice (i.e. arrow tip to the end of one head, back to an arrow tip, to the end of another head), and it results in a minor, but visible, change to the look of the arrowhead. It also changes the number of lines that are returned.

Thank you for your time!

timhoffm · 2024-03-01T19:26:48Z

As a quick confirmation, without the time to look into the code: This approach is to be preferred over the single line due to back compatibility.

scottshambaugh · 2024-03-01T19:36:50Z

Hi @AlecVercruysse, these changes look good to me. Could you add another image test for this behavior so we catch future regressions? The one in your most recent comment looks like a good example.

scottshambaugh

Once the CI tests pass, this looks good to me for merge. Congratulations @AlecVercruysse on your first contribution to matplotlib, and thank you for implementing this fix! We welcome future improvements and hope to see more of you!

Once this is merged in, I'd recommend linking to this PR in a new stackoverflow answer, and saying which matplotlib version this will be available in.

AlecVercruysse · 2024-03-01T21:09:05Z

@scottshambaugh Thank you! To confirm, this will appear in 3.8.4?

scottshambaugh · 2024-03-01T21:19:15Z

Yes, this is a bugfix and has no API changes so I think this is appropriate for a patch release.

Update figure test image

…754-on-v3.8.x Backport PR #27754 on branch v3.8.x (fix quiver3d incorrect arrow colors)

fix quiver3d incorrect arrow colors

e1e1a32

github-actions bot added the topic: mplot3d label Feb 7, 2024

github-actions bot reviewed Feb 7, 2024

View reviewed changes

revert quiver3d normalization

c3d60d3

AlecVercruysse added 2 commits February 7, 2024 16:07

quiver: avoid divide by zero when normalizing

f873277

- (avoid unnecessary computation otherwise)

Add tol to quiver3d & errorbar3d_errorevery image comparison test

c0b2442

add test for color coded quiver3d

0d5f822

scottshambaugh approved these changes Mar 1, 2024

View reviewed changes

scottshambaugh added this to the v3.8.4 milestone Mar 1, 2024

timhoffm approved these changes Mar 3, 2024

View reviewed changes

timhoffm merged commit 365c950 into matplotlib:main Mar 3, 2024

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 3, 2024

Backport PR matplotlib#27754: fix quiver3d incorrect arrow colors

788fbd6

meeseeksmachine mentioned this pull request Mar 3, 2024

Backport PR #27754 on branch v3.8.x (fix quiver3d incorrect arrow colors) #27849

Merged

dstansby pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 3, 2024

Backport PR matplotlib#27754: fix quiver3d incorrect arrow colors

6893bc8

Update figure test image

scottshambaugh mentioned this pull request Mar 3, 2024

Arrow head color fix to match arrow body #13742

Closed

6 tasks

ksunden added a commit that referenced this pull request Mar 6, 2024

Merge pull request #27849 from meeseeksmachine/auto-backport-of-pr-27…

4252b9f

…754-on-v3.8.x Backport PR #27754 on branch v3.8.x (fix quiver3d incorrect arrow colors)

scottshambaugh mentioned this pull request May 3, 2024

a more user-friendly way to specify color for each arrow in 3D quiver? #8484

Closed

scottshambaugh mentioned this pull request May 16, 2024

colormap support for 3d quiver #3382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix quiver3d incorrect arrow colors #27754

fix quiver3d incorrect arrow colors #27754

AlecVercruysse commented Feb 7, 2024 •

edited

Loading

github-actions bot left a comment

AlecVercruysse commented Feb 7, 2024

AlecVercruysse commented Feb 7, 2024

oscargus commented Feb 7, 2024

AlecVercruysse commented Feb 8, 2024

AlecVercruysse commented Mar 1, 2024

timhoffm commented Mar 1, 2024

scottshambaugh commented Mar 1, 2024 •

edited

Loading

scottshambaugh left a comment

AlecVercruysse commented Mar 1, 2024

scottshambaugh commented Mar 1, 2024 •

edited

Loading

fix quiver3d incorrect arrow colors #27754

fix quiver3d incorrect arrow colors #27754

Conversation

AlecVercruysse commented Feb 7, 2024 • edited Loading

PR summary

PR checklist

github-actions bot left a comment

Choose a reason for hiding this comment

AlecVercruysse commented Feb 7, 2024

AlecVercruysse commented Feb 7, 2024

oscargus commented Feb 7, 2024

AlecVercruysse commented Feb 8, 2024

AlecVercruysse commented Mar 1, 2024

timhoffm commented Mar 1, 2024

scottshambaugh commented Mar 1, 2024 • edited Loading

scottshambaugh left a comment

Choose a reason for hiding this comment

AlecVercruysse commented Mar 1, 2024

scottshambaugh commented Mar 1, 2024 • edited Loading

AlecVercruysse commented Feb 7, 2024 •

edited

Loading

scottshambaugh commented Mar 1, 2024 •

edited

Loading

scottshambaugh commented Mar 1, 2024 •

edited

Loading