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

Added arrowhead positioning options to quiver. #15396

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

PipGrylls
Copy link

@PipGrylls PipGrylls commented Oct 9, 2019

PR Summary

the optional keywords 'head_pos' has been added to quiver. 'head_pos' can put the arrowhead at the tip tail or middle of the arrow shaft dictated by keywords matching 'pivot', or be passed a float in range 0-1 to specify the exact location on the arrow shaft.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@@ -173,6 +174,15 @@

'mid' is a synonym for 'middle'.

head_pos : {'tail', 'mid', 'middle', 'tip'}, optional, default: 'tip'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest having a single head_pos argument that takes either a string or a float, with "tail" synonymous to 0, "middle" synonymous to 0.5, and tip synonymous to 1 (or reverse 0 and 1 -- it's actually not clear from the docstring which is which).
Also perhaps the pivot kwarg should also gain the same functionality?

Copy link
Author

Choose a reason for hiding this comment

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

I was following the style of pivot for similarity within the module. But in principle, I agree with you both should probably be more flexible. I am not sure however if the angles become significantly more complex if the pivot parameter is let loose.

Copy link
Author

Choose a reason for hiding this comment

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

I have made the edits to head_pos and committed changes I have not edited pivot but this could be opened as a feature request should people actually want it.

if 0.0 < mid_scale < 1.0:
self.mid_scale = mid_scale
else:
self.mid_scale = 0.5
Copy link
Contributor

@anntzer anntzer Oct 9, 2019

Choose a reason for hiding this comment

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

please error out in that case ("Errors should never pass silently.")
... or perhaps it actually makes sense to allow arrowheads outside of the shaft?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking about this perhaps they should be left to be as the user specified and throw instead a warning that the arrows will be off the shaft?
If I have more time I would also think about allowing this to be a 1D array to allow the arrowhead position on the shaft to convey extra information. This would be a reason for mid_scale and head_pos to stay independent I think as your other comment.

Copy link
Member

Choose a reason for hiding this comment

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

For now, I would just error out. We can always expand the functionality later on if there is demand.

Copy link
Author

Choose a reason for hiding this comment

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

I have made the edits and committed changes

@timhoffm
Copy link
Member

Please add E402 for quiver_head_pos_demo.py in .flake8.

@QuLogic
Copy link
Member

QuLogic commented Sep 25, 2020

The idea of this looks sound, and it doesn't appear that there's any opposition, but it seems to have been forgotten for a while. It now needs a rebase to fix conflicts, and there may be a few small typos to fix.

lib/matplotlib/quiver.py Show resolved Hide resolved
lib/matplotlib/quiver.py Outdated Show resolved Hide resolved
lib/matplotlib/quiver.py Outdated Show resolved Hide resolved
Comment on lines -473 to +484
self._axes = ax # The attr actually set by the Artist.axes property.
self.ax = ax
# modified quiver to allow for positioning of the arrowheads on the
# shaft
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change is intentional

@@ -487,6 +498,25 @@ def __init__(self, ax, *args,
self.angles = angles
self.width = width

# Checks the boundaries of mid_scale if outside range default of 0.5
if (head_pos == 0.0) or (head_pos == "tail"):
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to check both 0.0 and 'tail'. Instead, check for 'tail' (and the other strings), and set head_pos = 0.0 as appropriate. Then do a final check for 0.0 <= head_pos <= 1.0.

@@ -50,7 +50,8 @@
arrow more pointed, reduce *headwidth* or increase *headlength* and
*headaxislength*. To make the head smaller relative to the shaft,
scale down all the head parameters. You will probably do best to leave
minshaft alone.
minshaft alone. To set the position of the arrowhead used *head_pos* and
to exactly position the head on the shaft use *mid_scale*.
Copy link
Member

Choose a reason for hiding this comment

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

What is mid_scale? It doesn't appear to be part of this PR.

Y[:, 6] = np.ones(np.shape(Y[:, 6])) * 0.5
Y[:, 5:-1] *= -1
else:
raise ValueError("'head_pos' must be "
Copy link
Member

Choose a reason for hiding this comment

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

This has already been checked earlier, so no need to do so again.

PipGrylls and others added 6 commits September 28, 2020 07:43
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
self.head_pos = 1.0
elif (head_pos == "mid") or (head_pos == "middle"):
self.head_pos = 0.5
elif type(head_pos) == float:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you combine this into:

elif type(head_pos) == float and 0.0 < head_pos < 1.0:

then you only need the single final else to raise ValueError.

"or a string in {'tail', 'middle', 'tip'}")
else:
raise ValueError("'head_pos' must be "
"a value in the bounds 0head_pos<1,"
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "0head_pos<1,"

@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jun 28, 2023
@melissawm
Copy link
Contributor

Hi @PipGrylls - are you interested in picking this back up? If you need help with the rebase or moving forward, please let us know!

@PipGrylls
Copy link
Author

PipGrylls commented Jun 28, 2023 via email

@rcomer rcomer added status: orphaned PR and removed status: inactive Marked by the “Stale” Github Action labels Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

8 participants