-
Notifications
You must be signed in to change notification settings - Fork 18
Axis sharing fix #18
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
Axis sharing fix #18
Conversation
|
Note to self: not sure exactly how the sharing is operating at this moment. There was an api change that made the original implementation in proplot invalid and ultraplot inherited this legacy. The proposed change makes it work as it should, but the actual labels of the axis are still internally held the same. That is, we get a proper output image, but when I check if the labels of the individual axes are in fact shared, they are not. I feel uncomfortable merging this without this feature being there -- I will need to check if older versions in fact pass this test or not. |
|
Quick update.
|
|
Should be good now -- open for review |
|
Pinging @beckermr @pratiman-91 🔔 |
pratiman-91
left a comment
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.
It looks good to me! Maybe check with another reviewer.
beckermr
left a comment
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.
Relying on private class attributes is fragile. It'd be better to only use public methods / attributes.
|
The MPL documentation does not allow to share one axis across multiple subplots (see here). I tried implementing this: def _safe_share(self, axis_name, other_axis):
if hasattr(self, f"_share{axis_name}"):
setattr(self, f"_share{axis_name}", None)
getattr(self, f"share{axis_name}")(other_axis)However this results in the axis labels not being shared properly and the ticks being messed up: |
|
Note unsharing axis is also not allowed according to their docs. This would force us to either (a) write our own implementation, or abuse the api as is done here. |
|
Perhaps we could do this def share_axis(self, which, other):
if not isinstance(other, plot.PlotAxes):
raise ValueError("other must be an Axes instance")
shared = getattr(self, f"get_shared_{which}_axes")()
if which == "x":
if other in shared:
return
self._shared_axes["x"].join(self, other)
else:
if other in shared:
return
self._shared_axes["y"].join(self, other)To circumenvent the error and enable proper sharing? |
|
Then it appears developing an implementation using public apis is the correct approach. There appears to be api instability in MPL even for public attributes. One should therefore expect that relying explicitly on private ones will be even more unstable. I think the history of new MPL versions breaking with proplot/ultraplot matches these expectations. |
|
Actually scratch that these methods produce error. Not sure how to proceed here.. open for feedback. |
|
To maintain Ultraplot's multiple axis sharing capability while respecting Matplotlib's API changes, we can override Matplotlib's public sharing methods with our own implementation. This approach allows us to bypass Matplotlib's single-axis sharing limitation while preserving Ultraplot's existing functionality. Although not ideal, this serves as a pragmatic interim solution until a more permanent resolution can be implemented. |
beckermr
left a comment
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.
Comments on the code. I still see one private attribute accessed, but this PR is closer in that we are adding APIs that we needed and/or were removed.
Co-authored-by: Matthew R. Becker <beckermr@users.noreply.github.com>


Addresses #11 (comment)
The MPL api changed in v3.6 where get_[x/y]_shared_axes returns an immutable object.