-
-
Notifications
You must be signed in to change notification settings - Fork 7.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ba61fa8
to
3ed82d0
Compare
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, both hasattr(ax, "get_gridspec")
and
isinstance(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 like fig.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.
Fixing isinstance(ax, SubplotBase)
to make it work is relatively easy (via __instancecheck__
), and now done.
I can certainly also fix hasattr(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 does hasattr(ax, "get_gridspec")
, but what is the alternative for them? They can't get rid of the warning by doing ax.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) but fig.get_subplotspec(ax)
probably can (and then we can just wholly deprecate ax.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 the hasattr(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 the hasattr
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 would also be checking for None
? 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
to
aa40ea1
Compare
Most 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. |
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 argument rect
back and can save the additional rect 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 move self._subplotspec = None
down right before set_subplotspec
. self._subplotspec
is not yet needed and it's confusing to have two obviously related variables around.
I think that makes it clearer that subplotspec
is a temporary variable that gets modified down the line and self._subplotspec
will be updated from that through set_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 call set_subplotspec(spec)
on an absolute-positioned axes to convert it into a gridspec-positioned one, but set_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 with set_position
.
Also, perhaps better terminology for "gridspec-positioned" vs "absolute-positioned" could be useful for the docs.
31952d7
to
4b8b971
Compare
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 become pending 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. |
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 the Axes
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
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! |
@jklymak , thanks for the reply. Yes, we fixed the compatibility downstream at astropy/astropy#13880 . 😸 |
The matplotlib 'Axes' class no longer inherits from 'SubplotBase' since matplotlib/matplotlib#23573. This breaks the '_get_subplot' method in 'sampling_point.py' which checks if the passed 'axes_obj' is a 'SubplotBase' instance to distinguish 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 is and 'Axes' (as opposed to an array).
Bump matplotlib to ``3.7.1`` in ``poetry.lock``. The matplotlib 'Axes' class no longer inherits from 'SubplotBase' since matplotlib/matplotlib#23573. This breaks the '_get_subplot' method in 'sampling_point.py' which checks if the passed 'axes_obj' is a 'SubplotBase' instance to distinguish 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 is and '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 edits Note: 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 and CI/CD configurations. Upgrade mypy to avoid python/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' since matplotlib/matplotlib#23573. This breaks the '_get_subplot' method in 'sampling_point.py' which checks if the passed 'axes_obj' is a 'SubplotBase' instance to distinguish 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 is and '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 for compatibility with `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>
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 None
is 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.Also closes #11445 (by mostly getting rid of SubplotBase).
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and 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).