8000 Replace quiver dpi callback with reinit-on-dpi-changed. by anntzer · Pull Request #22807 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Replace quiver dpi callback with reinit-on-dpi-changed. #22807

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

Merged
merged 1 commit into from
Apr 9, 2022

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Apr 8, 2022

Instead of registering a callback to update itself whenever the dpi
changes, do the same thing as every other artist which depends on dpi
(e.g. FancyArrowPatch), i.e. update oneself as needed. We can still
save duplicate calculations by only doing the init if the figure dpi
changed compared to last time.

Also drop _new_UV, which is never read.

Closes #22804.

PR Summary

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.
  • 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).

Instead of registering a callback to update itself whenever the dpi
changes, do the same thing as every other artist which depends on dpi
(e.g. FancyArrowPatch), i.e. update oneself as needed.  We can still
save duplicate calculations by only doing the init if the figure dpi
changed compared to last time.

Also drop _new_UV, which is never read.
@greglucas
Copy link
Contributor

Nice! Thanks for the quick fix. I had just started looking into this as well and was going to change to using "resize_event" on the canvas which is what is really desired I think, but this is nicer :) On that note, I assume you realized that "dpi_changed" isn't used anywhere else in the codebase, so I would vote for deprecating that in another PR too... I think resize_event should be thrown from the dpi_update function because if you change the dpi you are really resizing the canvas?

@jklymak
Copy link
Member
jklymak commented Apr 8, 2022

Not looking at the details, but should these things be done in trans_dpi_acale?

@QuLogic
Copy link
Member
QuLogic commented Apr 9, 2022

I'm not sure that that would be possible as these are arrows positioned in data space, but whose size is determined by screen space.

@jklymak
Copy link
Member
jklymak commented Apr 9, 2022

We have this problem relatively often, at least with arrows - we want a glyph to have one dimension in data space, but the shape of the arrow heads in physical space. This doesn't seem too hard to do in physical space at draw time. Transfer the anchors to physical space and then draw the arrow and let its transform be physical space (for which we need a better name than trans_dpi_scale) if we do that dpi changes happen at the renderer level and the upper level code doesn't need a callback.

@jklymak
Copy link
Member
jklymak commented Apr 9, 2022

Is this only used for labelsep? Can we place text with an offset transform?

@@ -562,18 +525,19 @@ def _init(self):
"""
# It seems that there are not enough event notifications
# available to have this work on an as-needed basis at present.
if True: # not self._initialized:
if True: # self._dpi_at_last_init != self.axes.figure.dpi
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the comment? why does this code get run unconditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why the original check got commented out (and don't really want to investigate right now), I'm just replacing _initialized by its newer moral equivalent.


def _init(self):
if True: # not self._initialized:
if not self.Q._initialized:
if True: # self._dpi_at_last_init != self.axes.figure.dpi
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 understand why this condition is here if its always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above.

Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This seems good. I still think quiver should not have a concept of dpi, but I think this change could go in before a significant refactoring. Definitely prefer this to a callback.

have you tested changing the dpi? I'm not even sure how we do that.

@anntzer
Copy link
Contributor Author
anntzer commented Apr 9, 2022

Agree that deprecating dpi_changed is perhaps a good idea, but this pr is really just aiming for the minimum fix. Ditto for using offset_transform or dpi_scale_trans instead.
No tests either for now :) (if anyone wants to contribute one...)

Copy link
Contributor
@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I tested this on my multi-monitor mixed-dpi setup and it looks good to me. I think this is good to go for now and there are a few follow-up things that can be done in subsequent PRs.

@greglucas greglucas merged commit 1999228 into matplotlib:main Apr 9, 2022
@anntzer anntzer deleted the qd branch April 9, 2022 13:49
@greglucas greglucas mentioned this pull request Apr 9, 2022
1 task
@QuLogic QuLogic added this to the v3.5.2 milestone Apr 11, 2022
@QuLogic
Copy link
Member
QuLogic commented Apr 11, 2022

@meeseeksdev backport to v3.5.x

@lumberbot-app
Copy link
lumberbot-app bot commented Apr 11, 2022

Could not push to auto-backport-of-pr-22807-on-v3.5.x due to error, aborting.

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Apr 11, 2022
@QuLogic
Copy link
Member
QuLogic commented Apr 12, 2022

@meeseeksdev backport to v3.5.x

@lumberbot-app
Copy link
lumberbot-app bot commented Apr 12, 2022

Could not push to auto-backport-of-pr-22807-on-v3.5.x due to error, aborting.

@QuLogic
Copy link
Member
QuLogic commented Apr 13, 2022

meeseeksdev backport to v3.5.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Apr 13, 2022
oscargus added a commit that referenced this pull request Apr 13, 2022
…807-on-v3.5.x

Backport PR #22807 on branch v3.5.x (Replace quiver dpi callback with reinit-on-dpi-changed.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Quiver not working with subfigure?
4 participants
0