X Tutup
Skip to content

Add zorder option in QuiverKey#23116

Closed
muchojp wants to merge 12 commits intomatplotlib:mainfrom
muchojp:quiverkey_zorder
Closed

Add zorder option in QuiverKey#23116
muchojp wants to merge 12 commits intomatplotlib:mainfrom
muchojp:quiverkey_zorder

Conversation

@muchojp
Copy link
Copy Markdown
Contributor

@muchojp muchojp commented May 24, 2022 •
edited
Loading

PR Summary

Add zorder option in QuiverKey. So far, if you want to set zorder, you need set_zorder. Therefore, I can make zorder option available.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

github-actions[bot]
github-actions bot reviewed May 24, 2022
View reviewed changes
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. 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.

@jklymak jklymak mentioned this pull request May 24, 2022
Merged
6 tasks
@oscargus
Copy link
Copy Markdown
Member

oscargus commented May 24, 2022

Looks good! If you can please add a test for this it is helpful.

One way may be to use something like:

@check_figures_equal()
def test_zorder(fig_test, fig_ref):
    fig_test.quiver(QuiverKey(zorder=...))

   fig_ref.quiver(...)
    ..set_zorder(...)

(No idea about the exact syntax, but basically creating the same figure twice: one using the new keyword and one using the old set_zorder approach.)

This should go in tests/test_quiver.py and you will need to import check_figure_equal from matplotlib.testing.decorators.

@jklymak jklymak marked this pull request as draft May 24, 2022 10:53
@jklymak
Copy link
Copy Markdown
Member

jklymak commented May 24, 2022

feel free to move out of draft when there is a test. Feel free to ping if you need more guidance on the test. Note we already have test_quiver.py, which is probably the best place for this.

QuLogic
QuLogic reviewed May 24, 2022
View reviewed changes
@muchojp muchojp changed the title add zorder option in QuiverKey Add zorder option in QuiverKey May 30, 2022
@muchojp
Copy link
Copy Markdown
Contributor Author

muchojp commented May 30, 2022

Thank you for a lot of advice. I added a test and new API change doc.
Can you review this PR?

@muchojp muchojp marked this pull request as ready for review May 30, 2022 10:39
QuLogic
QuLogic reviewed Jun 21, 2022
View reviewed changes
lib/matplotlib/tests/test_quiver.py Outdated
X, Y, U, V = 1, 1, 2, 2

ax_test = fig_test.subplots()
zorder_value = 5.
Copy link
Copy Markdown
Member

@QuLogic QuLogic Jun 21, 2022

Choose a reason for hiding this comment

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

The default zorder for quiver is 1, so the default for quiverkey is 1.1. That means zorder=5 may effectively be the same as not specifying anything? So I think this test needs at least another artist in between 1.1 and 5 to confirm that the key really is moved up.

@jklymak jklymak marked this pull request as draft June 30, 2022 10:55
@jklymak jklymak marked this pull request as draft June 30, 2022 10:55
@jklymak jklymak marked this pull request as draft June 30, 2022 10:55
@jklymak
Copy link
Copy Markdown
Member

jklymak commented Jun 30, 2022

Moved back to draft until the test can be fixed. Thanks!

@rcomer
Copy link
Copy Markdown
Member

rcomer commented Mar 18, 2023

Hi @muchojp are you still interested in working on this?

muchojp and others added 6 commits June 4, 2024 14:59
@muchojp
add zorder option in QuiverKey
677bb49 
modify indentation

modify docstring
add zorder test

linting patch and small modify

Add new api changes doc
@muchojp
add zorder option in QuiverKey
d54e66a
@muchojp
modify docstring
3a4d612 
add zorder test
@muchojp
linting patch and small modify
39bbfbf
@muchojp @QuLogic
Update lib/matplotlib/tests/test_quiver.py
0d7c12d 
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@muchojp
add arg zorder
489786b
@muchojp muchojp force-pushed the quiverkey_zorder branch from 02ee5fc to 79a15a7 Compare June 4, 2024 15:08
@muchojp muchojp closed this Jun 4, 2024
@muchojp muchojp deleted the quiverkey_zorder branch June 4, 2024 23:14
@melissawm melissawm moved this from Waiting for author to Done in First Time Contributors Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuLogic QuLogic QuLogic left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees

No one assigned

Labels

API: consistency status: needs revision status: orphaned PR

Projects

First Time Contributors
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@muchojp @oscargus @jklymak @rcomer @QuLogic @melissawm
X Tutup