Add get/set methods for DPI in SubFigure

[scottjones03]

PR Summary

This fixes the following error:

matplotlib\lib\text.py line 1489, dop = self.figure.get_dpi()/72. AttributeError: 'SubFigure' object has no attribute 'get_dpi'.

Effect: in v3.5.2 it is not possible to save a figure with a subfigure to a PDF.

PR Checklist

Tests and Styling

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

Documentation

• [N/A ] 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).
• [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
• [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

[anntzer]

Should we really allow setting a subfigure's dpi (which actually affects the entire figure)? This is not so clear to me...

[jklymak]

Everywhere else we just use the getter: self.figure.dpi which FAICT works. So maybe just change the get_dpi? We should probably deprecate set/get_dpi so we don't have different implementations....

@anntzer artist.figure.dpi = ... just passes up to the parent.

I don't think users should ever be setting the dpi except on creation or output. but....

[github-actions]

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.

[scottjones03]

Everywhere else we just use the getter: self.figure.dpi which FAICT works. So maybe just change the get_dpi? We should probably deprecate set/get_dpi so we don't have different implementations....

@anntzer artist.figure.dpi = ... just passes up to the parent.

I don't think users should ever be setting the dpi except on creation or output. but....

I think it makes sense to deprecate the get/set methods at some point, but for now we should at least make it consistent with the Figures object as there could be more bugs related to this elsewhere.

[scottjones03]

Should we really allow setting a subfigure's dpi (which actually affects the entire figure)? This is not so clear to me...

This changes the parent Figure's dpi, so it wouldn't be a problem. Also, this is the current behaviour anyway as we allow subfig.dpi = ....

[anntzer]

@jklymak I'll happily defer to your judgment here, was just raising the point in case it was missed.

[jklymak]

Sure, we can add this (after you fix the flake8 lining errors)

However, please also consider changing the internal use of get/set. I think there are only about a dozen of them; that gives us a deprecation path. Thanks!

[scottjones03]

Sure, we can add this (after you fix the flake8 lining errors)

However, please also consider changing the internal use of get/set. I think there are only about a dozen of them; that gives us a deprecation path. Thanks!

Sure will take a look at it

[scottjones03]

Sure, we can add this (after you fix the flake8 lining errors)

However, please also consider changing the internal use of get/set. I think there are only about a dozen of them; that gives us a deprecation path. Thanks!

#23278

[oscargus]

I guess it would be good to add a smoke test (basically run the commands that now gives an error without actually checking the output). But maybe not worth it if this is just a temporary fix?

This should go in 3.5.3, right?

[tacaswell]

I would put this is 3.6 (new feature) and #23278 for 3.5.3 (bug fix by changing internal usage). It is a bit hair-splitting and I will defer to anyone who has a strong view on this.

[tacaswell]

The docs build failed on a git checkout problem, restarted.

[jklymak]

Subfigures are broken without this if certain artists are added to the subfigure, so I think it needs to be a bugfix

[scottjones03]

Subfigures are broken without this if certain artists are added to the subfigure, so I think it needs to be a bugfix

As mentioned, there is now a new PR that fixes this bug. This PR would now be a feature.

[scottjones03]

I guess it would be good to add a smoke test (basically run the commands that now gives an error without actually checking the output). But maybe not worth it if this is just a temporary fix?

This should go in 3.5.3, right?

#23278 would you rather this other PR be smoke tested? This one would not need a smoke test as it is no longer a bug fix.

[tacaswell]

This one would not need a smoke test as it is no longer a bug fix.

This probably should get an explicit test that the methods work as expected. Probably something like make a figure with subfigures, make sure that the get_dpi on the parent and children are consistent and that calling set_dpi on the child affects both the parent and its siblings.

[jklymak]

OK, but I think set_dpi and get_dpi should just be deprecated across the board, so not much need for a test?

[scottjones03]

OK, but I think set_dpi and get_dpi should just be deprecated across the board, so not much need for a test?

I kinda see your point. Given that set_dpi and get_dpi is deprecated internally from the other PR, we can decline this PR and have a case where we don't advertise to users that they can change the parent dpi from the child. The downside here is the inconsistency between the figure get/set methods and the subfigure lack of get/set methods could be buggy for users who accept both figure/subfigure types for a figure in their code (like it has been done internally).

[oscargus]

Should there be a self.stale = True here? (For symmetry with set_dpi?)

(Maybe not a question for you @scottjones03 though.)

[tacaswell]

Maybe, but in practice the parent will catch this and trigger the required re-draw.

[tacaswell]

set_dpi and get_dpi should just be deprecated across the board, so not much need for a test?

fair, but we are not deprecating them on Figure yet, we should try to make SubFigure behave as much like as possible, and when we lift the dpi tracking up a level, this will be far from the gnarliest thing we will have to deal with!.

[tacaswell]

Hi-five ✋ on merging your first pull request to Matplotlib, @scottjones03 ! We hope you stick around and invite you to continue to take an active part in Matplotlib!

Your choices aren’t limited to programming 😉 – you can review pull requests, help us stay on top of new and old issues, develop educational material, refresh our documentation, work on our website, create marketing materials, translate website content, write grant proposals, and help with other fundraising initiatives. For more info, check out: https://matplotlib.org/stable/devel/index

Also, consider joining our mailing list ✉️. This is a great way to connect with other people in our community and be part of important conversations that affect the development of Matplotlib.

If you haven’t yet, do join us on gitter and discourse 🗣. The former is a chat platform, which is great when you have any questions on process or how to fix something, the latter is a forum which is useful for longer questions and discussions.

Last but not least, we have a monthly meeting 👥 for new contributors and a weekly meeting for the maintainers, everyone is welcome to join both! You can find out more about our regular project meetings in this calendar page.