Skip to content

FIX: record original dpi when dpi set #24674

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
Closed
wants to merge 2 commits into from
Closed

FIX: record original dpi when dpi set #24674

wants to merge 2 commits into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Dec 9, 2022

PR Summary

Closes #24644 or at least some of it?

The figure._original_dpi is used when we want to print using dpi='figure'. However, if we manually set the dpi via figure.set_dpi this doesn't get set and the figure is saved at the origin figure dpi.

Example

import matplotlib.pyplot as plt

fig, ax = plt.subplots()
fig.set_dpi(150)
ax.plot([1, 2], [1, 2])
plt.show()
fig.savefig('Boo.png', dpi='figure')

On main, Boo.png has 100 dpi, whereas after this change it has 150 dpi.

Caveat.

This doesn't completely fix set_dpi, which doesn't respect deviceRatio when it is called, so hiDPI screens don't double the size and resolution of the figure.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@jklymak
Copy link
Member Author

jklymak commented Dec 9, 2022

Added test that dpi='figure' works as expected.

@tacaswell
Copy link
Member

This also leaves an inconsistency between fig.dpi = X and fig.set_dpi(X) (well, currently they are consistent in that neither works!).

@jklymak
Copy link
Member Author

jklymak commented Dec 9, 2022 •
edited
Loading

EDIT: Well again I would not let the user change either of these things post facto

However if you want this then we just need to make dpi a property and have a setter

@tacaswell
Copy link
Member

dpi is already a property, but we read/change/reset it internally so it is a bit messy.

Well again I would not let the user change either of these things post facto

Even if we drop the users ability to set it live, I think we would still want to keep the ability to set the dpi at save time to save multiple resolution versions of the same Figure.

@jklymak
Copy link
Member Author

jklymak commented Dec 9, 2022

Even if we drop the users ability to set it live, I think we would still want to keep the ability to set the dpi at save time to save multiple resolution versions of the same Figure.

Oh, definitely. I consider that creation time, as you are creating a new canvas.

@tacaswell
Copy link
Member

Oh, definitely. I consider that creation time, as you are creating a new canvas.

Not always. In Figure.savefig

matplotlib/lib/matplotlib/figure.py

Line 3273 in 1d2abe7

self.canvas.print_figure(fname, **kwargs)

which leads to

matplotlib/lib/matplotlib/backend_bases.py

Lines 2304 to 2306 in 1d2abe7

with cbook._setattr_cm(self, manager=None), \
self._switch_canvas_and_return_print_method(format, backend) \
as print_method, \

which leads to

matplotlib/lib/matplotlib/backend_bases.py

Lines 2201 to 2207 in 1d2abe7

elif hasattr(self, f"print_{fmt}"):
# Return the current canvas if it supports the requested format.
canvas = self
canvas_class = None # Skip call to switch_backends.
else:
# Return a default canvas for the requested format, if it exists.
canvas_class = get_registered_canvas_class(fmt)
so if we already are using a canvas that can output the format we want we just re-use it

@jklymak
Copy link
Member Author

jklymak commented Dec 9, 2022

so if we already are using a canvas that can output the format we want we just re-use it

To me, if you are changing the dpi you are essentially making a new canvas. I guess there is a fast path if you don't change the dpi, but that strikes me as so rare as to almost never happen, particularly now in the hiDPI era, where even the figure dpi is not the canvas dpi.

@QuLogic
Copy link
Member

QuLogic commented Dec 10, 2022

I'm beginning to remember why I didn't finish doing this fix in this manner. The problem as implemented here is that it doesn't work for HiDPI screens. In that case, the _original_dpi should be set to the input, and saved DPI should be input * scale. However, that does leave the inconsistency of the .dpi property not working. And if you make that consistent with set_dpi, then suddenly you need to make sure all the temporary DPI changes (e.g., for mixed-mode or for savefig with an explicit DPI) don't change _original_dpi or restore it correctly. So I gave up on that to pursue the move of the HiDPI handling entirely as we discussed in other places.

@ifurther ifurther mentioned this pull request Jan 4, 2023
Open
@jklymak
Copy link
Member Author

jklymak commented Jan 10, 2023

I guess I take it from above that we should close this. I'm not quite sure I follow the hiDPI argument - my test machine is high dpi and this works fine, but we can re-open if we decide we want it

@jklymak jklymak closed this Jan 10, 2023
@jklymak jklymak deleted the fix-set-dpi-printing branch January 10, 2023 21:42
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: fig.set_dpi() and figure.dpi option in matplotlibrc file not working
3 participants
@jklymak @tacaswell @QuLogic