Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Merge SubplotBase into AxesBase.#23573
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ba61fa8 to3ed82d0Compare| Following this change, checking ``hasattr(ax, "get_gridspec")`` or | ||
| ``isinstance(ax, SubplotBase)`` should now be replaced by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm +1 of getting rid of the separate Subplot classes.
However, if I understand this correctly, bothhasattr(ax, "get_gridspec") andisinstance(ax, SubplotBase) change behavior under this PR in that they are True also for non-suplots. This is an API break, we cannot afford without deprecation. There are ~600 matches / ~1700 matches on github (some in Matplotlib, but also in other libs).
We may also consider whether the subplot info needs to be stored / be accessible at all from the Axes. I'm on holidays, so not able to check code details, but it may be feasible to hold the arrangement info outside of the Axes. The arrangement is basically a figure-level concept. We could make it accessible from there and query it for information regarding a specific Axes, i.e. something likefig.get_gridspec().get_subplotspec(ax).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixingisinstance(ax, SubplotBase) to make it work is relatively easy (via__instancecheck__), and now done.
I can certainly also fixhasattr(ax, "get_gridspec") to make that return False for absolute-positioned axes, but that basically depends on what you think the end plan should be. Do you want to keep the hasattr check "for ever"? If not, I can't see a proper deprecation path: certainly, tricks based on__getattribute__ or@property can let me raise a DeprecationWarning whenever one doeshasattr(ax, "get_gridspec"), but what is the alternative for them? They can't get rid of the warning by doingax.get_gridspec() is None instead, because this will always also raise the DeprecationWarning due to how hasattr works. Keeping the hasattr check working forever seems slightly inelegant though (it won't be a normal method, for example).fig.get_gridspec().get_subplotspec(ax) probably can't be made to work (as you can have multiple gridspecs per figure) butfig.get_subplotspec(ax) probably can (and then we can just wholly deprecateax.get_gridspec/ax.get_subplotspec in all cases), but that's a bigger refactor and probably@jklymak should comment on that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think always having a get_gridspec in the base axes class that returns None if the axes is not part of a gridspec makes good sense.
I haven't researched this, but my feeling is that thehasattr(ax, 'get_gridspec') kicking around were simply kludges to test if an axes is part of a gridspec, but having a proper test seems better. I guess there are two breakages: if thehasattr passes and then downstream tries to do gridspec things on the gridspec that will immediately fail because the gridspec will be None. I would think most places where this check exists wouldalso be checking forNone? I'm also not sure we could remove the hasattr in case there are axes out there that do not use our base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree the breakage will typically be loud anyways (i.e. even if the hasattr check incorrectly succeeds, a later stage will likely fail).
I don't think an "axes out there that does not use our base class" is really likely to exist at all -- there are far too many private attributes that need to be set up correctly for that to be realistic...
54561a7 toaa40ea1CompareMost missing coverage should be fixed now; I think everything still missing was already not covered before and just slightly shifted around so I'll skip them.... |
|
Rebased to fix that. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| rect : tuple (left, bottom, width, height). | ||
| The Axes is built in the rectangle *rect*. *rect* is in | ||
| `.Figure` coordinates. | ||
| *args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this needed to unite the Axes and Subplot constuctor APIs? That's quite ugly. We should consider to migrate three numbers to "one tuple of three numbers" so that we have always exactly one argumentrect back and can save the additionalrect in kwargs handling.
But that may be for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this is super ugly and I do want to move to "one tuple of 3 numbers", but that cannot be done with yet another deprecation cycle which can be done later.
lib/matplotlib/axes/_base.py Outdated
| rect = kwargs.pop("rect") | ||
| _api.check_isinstance((mtransforms.Bbox, Iterable), rect=rect) | ||
| args = (rect,) | ||
| self._subplotspec = subplotspec = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| self._subplotspec=subplotspec=None | |
| subplotspec=None |
and moveself._subplotspec = None down right beforeset_subplotspec.self._subplotspec is not yet needed and it's confusing to have two obviously related variables around.
I think that makes it clearer thatsubplotspec is a temporary variable that gets modified down the line andself._subplotspec will be updated from that throughset_subplotspec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure. This does by the way raise the following point: right now (with this PR) it is possible to callset_subplotspec(spec) on an absolute-positioned axes to convert it into a gridspec-positioned one, butset_subplotspec(None) won't work. Perhaps we should try to make it work too, though we'd need to figure out exactly how it interacts withset_position.
Also, perhaps better terminology for "gridspec-positioned" vs "absolute-positioned" could be useful for the docs.
31952d7 to4b8b971Compare| from ._axes import * | ||
| # Backcompat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO we should get rid of this stuff eventually.
As a first step, they should becomepending deprecated and we should communicate that people should move away from subplot stuff in the change notes.
Do we want to deprecate this stuff?
| rectangle or a single `.Bbox`. This specifies the rectangle (in | ||
| figure coordinates) where the Axes is positioned. | ||
| ``*args`` can also consist of three numbers or a single three-digit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should move away from three separate number parameters towards one tuple, so that we can have a single parameter again. I think this should not be too annoying downstream because few people instantiate Axes directly.
This would need a pending deprecation. On the downside, we'd have for the one parameter a 4-tuple meaning a rect and a 3-tuple meaning a subplot spec, but that seems bearable.
I think the change here is quite large and somewhat likely to cause breakage when it is first released; I would thus suggest first making sure that everything works before starting to deprecate the old API (which I agree should happen at some point). This will avoid e.g. forcing third-parties to blacklist a future 3.7.0 because it accidentally offers no way to do something previously possible without triggering deprecationwarnings (or requires large contorsions to do so). The old API has basically been around since "forever"; deprecating it in 3.8 instead of 3.7 doesn't seem too bad. Even if you really want to put in the deprecation in 3.7, we can always do that later in the dev cycle, when we've had a bit of matplotlib-internal devel to make sure I didn't miss anything. |
timhoffm left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good. This is so fundmental, that it's very well possible it breaks somebody. OTOH, getting rid of Subplots is an important simplification, which is worth the risk. I don't have any additional ideas for more compatibility/safeguards than what is already implemented here. Let's get this in early in the 3.7 dev phase, so that there are enough chances to stumble over obstacles before a release.
Agreed, that we should not deprecate anything yet (or even for the 3.7 release).
This should be good to go for 3.7 now. |
Thank you@anntzer , lets see what this breaks :) |
| - ``get_geometry`` (use ``SubplotBase.get_subplotspec`` instead), | ||
| - ``change_geometry`` (use ``SubplotBase.set_subplotspec`` instead), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The removal message in 3.6 API changes was not updated like here, which is breaking docs. But should these alternatives really be made code-style instead of linking to theAxes methods instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair question. I consider the changelog a historic document that reflects the state at that time. We also don't go back and change other things in changelog, that are not correct anymore. So, code-style is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK,#24236
pllim commentedOct 21, 2022
Hello. I think this broke |
@pllim it looks like astropy resolved this on their end? If not, please open a bug report. Thanks for testing master! |
pllim commentedOct 23, 2022
@jklymak , thanks for the reply. Yes, we fixed the compatibility downstream atastropy/astropy#13880 . 😸 |
The matplotlib 'Axes' class no longer inherits from 'SubplotBase'sincematplotlib/matplotlib#23573.This breaks the '_get_subplot' method in 'sampling_point.py' whichchecks if the passed 'axes_obj' is a 'SubplotBase' instance todistinguish between indexable and non-indexable objects.To fix this, we now use a try-except block to index into the'axes_obj', and catch the 'TypeError' that is raised if it isand 'Axes' (as opposed to an array).
Bump matplotlib to ``3.7.1`` in ``poetry.lock``.The matplotlib 'Axes' class no longer inherits from 'SubplotBase'sincematplotlib/matplotlib#23573.This breaks the '_get_subplot' method in 'sampling_point.py' whichchecks if the passed 'axes_obj' is a 'SubplotBase' instance todistinguish between indexable and non-indexable objects.To fix this, we now use a try-except block to index into the'axes_obj', and catch the 'TypeError' that is raised if it isand 'Axes' (as opposed to an array).---------Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>Co-authored-by: Dominik Gresch <dominik.gresch@ansys.com>
* Bump ansys-sphinx-theme from 0.9.5 to 0.9.6 (#240)Bumps [ansys-sphinx-theme](https://github.com/ansys/ansys-sphinx-theme) from 0.9.5 to 0.9.6.- [Release notes](https://github.com/ansys/ansys-sphinx-theme/releases)- [Commits](ansys/ansys-sphinx-theme@v0.9.5...v0.9.6)---updated-dependencies:- dependency-name: ansys-sphinx-theme dependency-type: direct:production update-type: version-update:semver-patch...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Remove the poetry workaround for sphinx theme installation (#241)* Update dpf core version (#243)* Bump peter-evans/create-or-update-comment from 2 to 3 (#249)Bumps [peter-evans/create-or-update-comment](https://github.com/peter-evans/create-or-update-comment) from 2 to 3.- [Release notes](https://github.com/peter-evans/create-or-update-comment/releases)- [Commits](peter-evans/create-or-update-comment@v2...v3)---updated-dependencies:- dependency-name: peter-evans/create-or-update-comment dependency-type: direct:production update-type: version-update:semver-major...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Bump pytest from 7.2.2 to 7.3.0 (#250)Bumps [pytest](https://github.com/pytest-dev/pytest) from 7.2.2 to 7.3.0.- [Release notes](https://github.com/pytest-dev/pytest/releases)- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)- [Commits](pytest-dev/pytest@7.2.2...7.3.0)---updated-dependencies:- dependency-name: pytest dependency-type: direct:production update-type: version-update:semver-minor...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Short fiber example: fiber orientation tensor (#233)* Bump ansys-dpf-core from 0.8.0 to 0.8.1 (#253)Bumps [ansys-dpf-core](https://github.com/pyansys/pydpf-core) from 0.8.0 to 0.8.1.- [Release notes](https://github.com/pyansys/pydpf-core/releases)- [Commits](ansys/pydpf-core@v0.8.0...v0.8.1)---updated-dependencies:- dependency-name: ansys-dpf-core dependency-type: direct:production update-type: version-update:semver-patch...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Bump pytest from 7.3.0 to 7.3.1 (#254)Bumps [pytest](https://github.com/pytest-dev/pytest) from 7.3.0 to 7.3.1.- [Release notes](https://github.com/pytest-dev/pytest/releases)- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)- [Commits](pytest-dev/pytest@7.3.0...7.3.1)---updated-dependencies:- dependency-name: pytest dependency-type: direct:production update-type: version-update:semver-patch...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Add info to explain switching of doc versions (#256)Update main index.rst file with minor editsNote: The display links to other topics should use the same title as the topic. This is a Google developer doc style guideline. Users can be confident that the have been taked to the correct place.* Drop support for Python 3.7 (#257)Drop support for Python 3.7, and remove it from the tox andCI/CD configurations.Upgrade mypy to avoidpython/mypy#13499,and add checks for 'None' to account for newly reported errors.* Bump pre-commit from 2.21.0 to 3.2.2 (#259)Bumps [pre-commit](https://github.com/pre-commit/pre-commit) from 2.21.0 to 3.2.2.- [Release notes](https://github.com/pre-commit/pre-commit/releases)- [Changelog](https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md)- [Commits](pre-commit/pre-commit@v2.21.0...v3.2.2)---updated-dependencies:- dependency-name: pre-commit dependency-type: direct:production update-type: version-update:semver-major...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Update AWP_ROOT_DOCKER variable (#265)* Update pre-commit dependencies, remove code obsolete with Python 3.7 drop (#258)* Update pre-commit dependencies.* Add and run the `pyupgrade` pre-commit hook.* Remove `TYPE_CHECKING` guards for `numpy.typing`.* Set the minimum requirement for `numpy` to `1.22`.* Add 'build-wheelhouse' job to CI/CD (#264)* Add 'build-wheelhouse' job to CI/CD* Rename build-package to build-library, improve dependencies* Add allow-last-days option to package cleanup* Bump ansys-sphinx-theme from 0.9.7 to 0.9.8 (#266)Bumps [ansys-sphinx-theme](https://github.com/ansys/ansys-sphinx-theme) from 0.9.7 to 0.9.8.- [Release notes](https://github.com/ansys/ansys-sphinx-theme/releases)- [Commits](ansys/ansys-sphinx-theme@v0.9.7...v0.9.8)---updated-dependencies:- dependency-name: ansys-sphinx-theme dependency-type: direct:production update-type: version-update:semver-patch...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>Co-authored-by: janvonrickenbach <jan.vonrickenbach@ansys.com>* Bump numpy from 1.22.0 to 1.24.3 (#267)Bumps [numpy](https://github.com/numpy/numpy) from 1.22.0 to 1.24.3.- [Release notes](https://github.com/numpy/numpy/releases)- [Changelog](https://github.com/numpy/numpy/blob/main/doc/RELEASE_WALKTHROUGH.rst)- [Commits](numpy/numpy@v1.22.0...v1.24.3)---updated-dependencies:- dependency-name: numpy dependency-type: direct:production update-type: version-update:semver-minor...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>Co-authored-by: janvonrickenbach <jan.vonrickenbach@ansys.com>* Bump pre-commit from 3.2.2 to 3.3.1 (#271)Bumps [pre-commit](https://github.com/pre-commit/pre-commit) from 3.2.2 to 3.3.1.- [Release notes](https://github.com/pre-commit/pre-commit/releases)- [Changelog](https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md)- [Commits](pre-commit/pre-commit@v3.2.2...v3.3.1)---updated-dependencies:- dependency-name: pre-commit dependency-type: direct:production update-type: version-update:semver-minor...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Bump ansys-sphinx-theme from 0.9.8 to 0.9.9 (#273)Bumps [ansys-sphinx-theme](https://github.com/ansys/ansys-sphinx-theme) from 0.9.8 to 0.9.9.- [Release notes](https://github.com/ansys/ansys-sphinx-theme/releases)- [Commits](ansys/ansys-sphinx-theme@v0.9.8...v0.9.9)---updated-dependencies:- dependency-name: ansys-sphinx-theme dependency-type: direct:production update-type: version-update:semver-patch...Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>* Adapt to organization rename from 'pyansys' to 'ansys' (#276)Change the following references:- `github.com/pyansys` -> `github.com/ansys`- `ghcr.io/pyansys` -> `ghcr.io/ansys`- `pyansys/actions` -> `ansys/actions`- `https://codecov.io/gh/pyansys/pydpf-composites` -> `https://codecov.io/gh/ansys/pydpf-composites`* Adapt to change in Axes with maptlotlib==3.7.1 (#263)Bump matplotlib to ``3.7.1`` in ``poetry.lock``.The matplotlib 'Axes' class no longer inherits from 'SubplotBase'sincematplotlib/matplotlib#23573.This breaks the '_get_subplot' method in 'sampling_point.py' whichchecks if the passed 'axes_obj' is a 'SubplotBase' instance todistinguish between indexable and non-indexable objects.To fix this, we now use a try-except block to index into the'axes_obj', and catch the 'TypeError' that is raised if it isand 'Axes' (as opposed to an array).---------Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>Co-authored-by: Dominik Gresch <dominik.gresch@ansys.com>* Relax pyvista requirement upper bound (#278)Allow all pyvista versions `<1` to be used. This is required forcompatibilitywith `ansys-fluent-visualization`, which depends on `>=0.39.0`.* Document docker container from customer portal (#277)* Support customer portal docker images and update docs* Remove --server-bin pytest option because it is not working anymore.* Ignore generated result definitions.* Update README.rst* Automatically upload file to server in CompositeModel* Add 2024_1_pre0 to supported directories.* Add show function to show sampling point plots---------Co-authored-by: Dominik Gresch <greschd@users.noreply.github.com>---------Signed-off-by: dependabot[bot] <support@github.com>Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>Co-authored-by: Dominik Gresch <greschd@users.noreply.github.com>Co-authored-by: Federico Negri <FedericoNegri@users.noreply.github.com>Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>Co-authored-by: Dominik Gresch <dominik.gresch@ansys.com>
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
See discussion at#18222 (which this finally closes "for real") and#18564. This intentionally does not deprecate yet the subplot-related API, which can be done in a followup PR,if we decide to really do it (possibly some of the APIs have been around for really long and should just be kept around as empty shells as they don't cost much).
Note that if we decide that even the change from
hasattr(ax, "get_subplotspec")toax.get_subplotspec() is not Noneis too much, we could still keep most of this PR and make e.g.Axes.get_subplotspec = property(lambda self: lambda _: self._subplotspec), so that the hasattr check still returns False for non-gridspec-positioned axes.Alsocloses#11445 (by mostly getting rid of SubplotBase).
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).