Skip to content

FIX: Disallow twinx/twiny on Axes3D#31524

Merged
QuLogic merged 1 commit intomatplotlib:mainfrom
timhoffm:doc-3d-not-implemented
Apr 21, 2026
Merged

FIX: Disallow twinx/twiny on Axes3D#31524
QuLogic merged 1 commit intomatplotlib:mainfrom
timhoffm:doc-3d-not-implemented

Conversation

@timhoffm
Copy link
Copy Markdown
Member

@timhoffm timhoffm commented Apr 18, 2026

Closes #31523.

The root cause is that Axes3D inherits a lot of functionality from Axes that does not work for 3D. This is a design flaw we cannot easily fix.

With this PR we have at least a reasonable mechanism in place to flag not-supported methods. When we get aware of additional methods, we can easily add them.

@rcomer
Copy link
Copy Markdown
Member

rcomer commented Apr 18, 2026

From the note in the NotImplementedError docs, I think we should actually be setting these methods to None.
https://docs.python.org/3/library/exceptions.html#NotImplementedError

@timhoffm
Copy link
Copy Markdown
Member Author

I acknowledge the Python reference docs. However, setting to None has a non-helpful runtime behavior: "TypeError: 'NoneType' object is not callable".

I've followed the pattern of the semilog errors right below, because I think that's decent. You may debate whether NotImplementedError is exactly the right type, and we could change to something else like RuntimeError if you feel stongly. But I think NotImplementedError is bearable given the docs also state "indicate that the real implementation still needs to be added." - in theory we could add that functionality even though its not planned.

@buddy0452004
Copy link
Copy Markdown
Contributor

I tested both approaches and wanted to share a finding that might be useful for this discussion.
The None approach has a hidden UX problem. Axes3D already uses = None for get_frame_on and set_frame_on so @rcomer suggestion is consistent with the existing codebase pattern. However, when a user actually calls a None overridden method, they still get a cryptic error:

ax.get_frame_on()
# TypeError: 'NoneType' object is not callable

This is essentially the same problem we're trying to fix just a different cryptic error. Setting twinx = None would give users TypeError: 'NoneType' object is not callable instead of the current AttributeError: 'YAxis' object has no attribute 'tick_left'. Neither tells the user why it failed.

The NotImplementedError approach is better UX it gives a clear, actionable message. The Python docs note about NotImplementedError is about abstract base classes specifically, not a general recommendation against using it to signal unsupported operations.

My suggestion: If we go with None for consistency with the existing pattern, we should also fix get_frame_on and set_frame_on to use NotImplementedError in a follow-up otherwise we're perpetuating the same bad UX pattern. If we go with NotImplementedError here, it becomes the new standard for unsupported methods in Axes3D going forward.

Either way, this PR is a clear improvement over the current behaviour. Just wanted to surface this so the decision is made intentionally.

@buddy0452004
Copy link
Copy Markdown
Contributor

buddy0452004 commented Apr 18, 2026

I also noticed get_frame_on and set_frame_on in Axes3D are currently set to None would it make sense to migrate those to the new partialmethod pattern in a follow-up PR for consistency?

@rcomer
Copy link
Copy Markdown
Member

rcomer commented Apr 18, 2026

Ah OK, I admit I didn’t actually try it 🐑, just assumed that the documented recommendation would give something useful.

A more specific error might be good: something like SubclassUnsupportedError, but I don’t feel strongly.

@timhoffm timhoffm force-pushed the doc-3d-not-implemented branch from bc26389 to c29bc0c Compare April 18, 2026 21:41
@github-actions github-actions Bot added the Documentation: API files in lib/ and doc/api label Apr 18, 2026
Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated
from . import axis3d


class UnsupportedError(RuntimeError):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the issue, @buddy0452004 noted that Polar has a similar problem, so I’m wondering if the exception should be defined somewhere more general that both subclass modules can import from.

Or is toolkits considered sufficiently separate that it makes sense to have its own exceptions?

@buddy0452004
Copy link
Copy Markdown
Contributor

Since mpl_toolkits and matplotlib are separate packages (different install roots), defining UnsupportedError inside mpl_toolkits.mplot3d and importing it from matplotlib.projections.polar would create a cross-package dependency which feels wrong architecturally.

The natural shared home would be matplotlib.axes._base (or a new matplotlib.exceptions module), since both Axes3D and PolarAxes inherit from _AxesBase. That way:

  • Axes3D imports from matplotlib.axes._base (already does this heavily)
  • PolarAxes imports from matplotlib.axes._base (same)
  • No cross-toolkit dependency
  • The exception is co-located with the base class that defines the contract

I confirmed polar has the same issue ax.twinx() on a polar axes raises ValueError: Cannot set Axes adjustable to 'datalim' for Axes which override 'get_data_ratio' another cryptic internal error. So the fix would benefit both projections.

If it's useful, I'm happy to look into adding UnsupportedError to _AxesBase and applying the partialmethod pattern to PolarAxes in a follow-up PR.

@timhoffm timhoffm force-pushed the doc-3d-not-implemented branch from c29bc0c to b596d7e Compare April 19, 2026 22:38
@timhoffm
Copy link
Copy Markdown
Member Author

Ok, since this is needed more often, it should live in matplotlib._api. I've also created a descriptor so that it's easier to mark functions as unsupported:

twinx = _api.unsupported_method()

Note: I've removed the explicit tests for the unsupported log functions, since the descriptor itself is tested and we therefore don't need to test every application.

@timhoffm timhoffm force-pushed the doc-3d-not-implemented branch 2 times, most recently from ec9cd6e to 0225841 Compare April 19, 2026 22:47
Closes matplotlib#31522.

The root cause is that Axes3D inherits a lot of functionality from Axes
that does not work for 3D. This is a design flaw we cannot easily fix.

With this PR we have at least a reasonable mechanism in place to flag
not-supported methods. When we get aware of additional methods, we can
easily add them.
@timhoffm timhoffm force-pushed the doc-3d-not-implemented branch from 0225841 to f8a68b7 Compare April 20, 2026 00:07
@github-actions github-actions Bot removed the Documentation: API files in lib/ and doc/api label Apr 20, 2026
@buddy0452004
Copy link
Copy Markdown
Contributor

Great solution placing it in matplotlib._api makes it reusable across the whole codebase. Thanks for the thorough fix

Copy link
Copy Markdown
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

Nice! I didn’t know about __set_name__ 👀

@QuLogic QuLogic merged commit 44a9476 into matplotlib:main Apr 21, 2026
41 checks passed
@QuLogic QuLogic added this to the v3.11.0 milestone Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: twinx() and twiny() crash with cryptic errors on 3D axes

4 participants