Skip to content

Add public API for setting an axis unit converter #19229

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.

Sign up for GitHub

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

Jump to bottom
Closed
mwaskom opened this issue Jan 3, 2021 · 5 comments · Fixed by #28970
Closed

Add public API for setting an axis unit converter #19229

mwaskom opened this issue Jan 3, 2021 · 5 comments · Fixed by #28970
Assignees
@tacaswell @ksunden
Labels
keep Items to be ignored by the “Stale” Github Action New feature topic: units and array ducktypes
Milestone
v3.10.0

Comments

@mwaskom
Copy link

mwaskom commented Jan 3, 2021 •
edited
Loading

Problem

I would like to define and use a custom unit converter class. The existing mpl.units.registry system is type-based, which does not work for my usecase. I need my converter to generalize over types. But I also know exactly which (existing) axis objects should use the converter.

The closest I can get in associating a custom converter with an existing axis, following the code in axis.update_units is

ax = plt.figure().subplots()
ax.xaxis.converter = CustomConverter()
ax.xaxis._update_axisinfo()
ax.xaxis.stale = True

Proposed Solution

A public axis.set_converter API would solve this problem, e.g.

ax = plt.figure().subplots()
ax.set_converter(CustomConverter())

The idea is that this would basically do what Axis.update_units does, but consuming a ConversionInterface rather than consuming data and then getting a converter from the registry.

Additional context and prior art

The other thing I would mention is that I could probably work-around this problem if the registry lookup were based on the container dtype rather than the type of the individual elements (as is currently the case). But that seems to complexify the existing API, whereas the this proposal would add an orthogonal API, and it is probably better for that reason.
@tacaswell tacaswell added this to the v3.5.0 milestone Jan 3, 2021
@tacaswell
Copy link
Member

This would not be enough as #10206 added a cut-out (which was refactored a bit in #13738 and #14363) to not try and convert numbers.

In addition to adding set_converter would would have to extend the public API of the converters to at the instance level bail out of trying to do the conversion. It looks like we could do this by over-riding is_numlike on sub-classes, but then we can not defer the "check if we need to predict a converter" step (which has been in the code base from 84ede56 )

@jklymak
Copy link
Member

jklymak commented Jan 4, 2021

I think we could remove the cutout and make dealing with numbers the prerogative of the converter?

@mwaskom mwaskom mentioned this issue Jan 7, 2021
Merged
@mwaskom
Copy link
Author

mwaskom commented Jan 8, 2021

FYI I've decided to solve this in seaborn by internally coercing data to strings in situations where it should receive "categorical" treatment. I I think, for now at least, I don't need any changes in matplotlib to support this particular use-case. I'm going to close the issue, but if you think this is a good idea independently of my narrow problem, feel free to re-open.

@mwaskom mwaskom closed this as completed Jan 8, 2021
@jklymak
Copy link
Member

jklymak commented Jan 8, 2021

I think we should keep this open for the upcoming unit work.

@jklymak jklymak reopened this Jan 8, 2021
@tacaswell tacaswell self-assigned this May 14, 2021
@tacaswell tacaswell modified the milestones: v3.5.0, v3.6.0 Aug 5, 2021
@QuLogic QuLogic modified the milestones: v3.6.0, unassigned Jul 8, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions GitHub Actions
Copy link

This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help!

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Nov 13, 2023
@tacaswell tacaswell modified the milestones: future releases, v3.9.0 Nov 13, 2023
@tacaswell tacaswell added keep Items to be ignored by the “Stale” Github Action and removed status: inactive Marked by the “Stale” Github Action labels Nov 13, 2023
@tacaswell tacaswell modified the milestones: v3.9.0, v3.10.0 Mar 13, 2024
@ksunden ksunden mentioned this issue Oct 11, 2024
Merged
5 tasks
ksunden added a commit to ksunden/matplotlib that referenced this issue Oct 11, 2024
@ksunden
Add explicit converter setting to Axis
12f4078 
Closes matplotlib#19229
tacaswell pushed a commit that referenced this issue Oct 31, 2024
@ksunden
Add explicit converter setting to Axis (#28970)
32ec60d 
* Add explicit converter setting to Axis

Closes #19229


The replacement is the get/set_converter method.

This includes changes to use the getter and the private setter in the qt
figure options dialog menu.

The choice to use the private setter was a defensive one as the public
setter prevents being called multiple times (though does short circuit
if an identical input is provided, which I think is actually true here,
therefore the public one is probably functional (and a no-op).)

It is not clear to me on analysis how the unit information is or was
lost. A quick test commenting out the two lines which reset
converter/units displayed no obvious detrimental effect to removing
those, suggesting that even if once they were necessary, they may no
longer be.

These lines were last touched in #24141, though that really only generalized
the code into a loop rather than copy/pasted x and y behavior.
The original inclusion of resetting was in #4909, which indicated that
the dialog reset unit info. AFAICT, that is no longer true, though I
have not rigorously proved that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tacaswell tacaswell

@ksunden ksunden

Labels
keep Items to be ignored by the “Stale” Github Action New feature topic: units and array ducktypes
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging a pull request may close this issue.

Add explicit converter setting to Axis ksunden/matplotlib
6 participants
@tacaswell @QuLogic @mwaskom @story645 @jklymak @ksunden