I'd rather suggest patching ax.format_xdata and ax.format_ydata directly (I'm not convinced there's much of a point in adding an extra layer). See also #5804...
The present API is confusing and hard to remember, but telling users to replace Axes instance methods is not very appealing to me; it seems like the sort of thing that should be minimized. Asking them to set the value of an attribute, as is done presently, seems more normal.
It's mostly a question of code philosophy, I think, and I am aware that patching methods is poorly regarded by many. I don't think there's much of a point in entering a long debate on the topic here so I'll just state that:
When the intent is (effectively) to allow users to fully replace the behavior of a method by the behavior of another callable, I believe it is clearer to document the method as patchable, rather than to say "if you set this other attribute to a callable, then we'll call it instead of the original implementation" (in which case the end user (or at least I, when I encounter such APIs...) 1. needs to remember there's a second attribute name and what it is, and 2. will wonder whether the replacement implementation is used in all cases, or whether there are actually cases where it's not used).
Patching the method is not quite the same as modifying its behavior via an attribute because the former blows away the original, complete with its docstring, while the latter allows one to easily return to the original.
But maybe what is really needed here is a friendlier API, a single method that can be used to control either or both of the cursor readout formats. Maybe format_readout. Deprecate the old API, and turn format_xdata, fmt_xdata (if still used) into private attributes.
When patching at the instance level, you can, in fact, retrieve the original method, simply by del'ing the instance level patch:
In [1]: class C:
...: def f(self): print("orig")
...: c = C()
...: c.f()
...: c.f = lambda: print("patched")
...: c.f()
...: del c.f
...: c.f()
...:
orig
patched
orig
(note that you cannot del a method defined on the class via an instance:
In [2]: del c.f
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
<ipython-input-2-1ecec44184da> in <module>()
----> 1 del c.f
AttributeError: f
)
While it is true that docstrings are lost, I think the docstring of a method such as Axes.format_xdata is really most used to generate rendered docs (or consulted with pydoc, etc.); it would seem quite rare to me to try to consult a docstring on a class that I'd be patching in the same interactive session (even via the fmt_xdata layer) (but what do I know...).
I'd suggest this could go in the toc of doc/users/interactive.rst?
I was going to agree w/ Eric that patching is obscure, but then I realized the only times I've had to chage the format was for moving between x,y and geographic co-ordinates with degrees minutes as dd mm.mmm , and that needs a function.
@jklymak Even though it doesn't really help my argument :-) I don't think that's really an issue, its should be exactly the same thing to patch fmt_xdata or format_xdata (or do you have an example?)
Ah, sure, you're talking about overriding another method (and in this specific case it seems a fairly legit use case (yes, I know, you could write a custom formatter and install it as the Axis formatter... but why would you)).
I guess there the argument is that it doesn't make sense to provide specific override points for each methods when you can just override the method itself.
I think we should document setting fmt_xdata and fmt_ydata (rather than document to replace format_xdata) and make the except in format_xdata broader to provide 'lead-underwear' (as one of my colleges at work says). These functions are called on the GUI event loops and should not raise (as that can kill Qt).
Unfortunately format_coord makes the assumption that the x/y values are separable so the only option is to completely replace it. However, that something the project sub-classes should probably just take care of.
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!
Activity
anntzer commented on Oct 27, 2017
I'd rather suggest patching ax.format_xdata and ax.format_ydata directly (I'm not convinced there's much of a point in adding an extra layer). See also #5804...
efiring commented on Oct 27, 2017
The present API is confusing and hard to remember, but telling users to replace Axes instance methods is not very appealing to me; it seems like the sort of thing that should be minimized. Asking them to set the value of an attribute, as is done presently, seems more normal.
anntzer commented on Oct 27, 2017
It's mostly a question of code philosophy, I think, and I am aware that patching methods is poorly regarded by many. I don't think there's much of a point in entering a long debate on the topic here so I'll just state that:
When the intent is (effectively) to allow users to fully replace the behavior of a method by the behavior of another callable, I believe it is clearer to document the method as patchable, rather than to say "if you set this other attribute to a callable, then we'll call it instead of the original implementation" (in which case the end user (or at least I, when I encounter such APIs...) 1. needs to remember there's a second attribute name and what it is, and 2. will wonder whether the replacement implementation is used in all cases, or whether there are actually cases where it's not used).
efiring commented on Oct 27, 2017
Patching the method is not quite the same as modifying its behavior via an attribute because the former blows away the original, complete with its docstring, while the latter allows one to easily return to the original.
But maybe what is really needed here is a friendlier API, a single method that can be used to control either or both of the cursor readout formats. Maybe format_readout. Deprecate the old API, and turn format_xdata, fmt_xdata (if still used) into private attributes.
anntzer commented on Oct 27, 2017
When patching at the instance level, you can, in fact, retrieve the original method, simply by del'ing the instance level patch:
In [1]: class C: ...: def f(self): print("orig") ...: c = C() ...: c.f() ...: c.f = lambda: print("patched") ...: c.f() ...: del c.f ...: c.f() ...: orig patched orig(note that you cannot del a method defined on the class via an instance:
)
While it is true that docstrings are lost, I think the docstring of a method such as Axes.format_xdata is really most used to generate rendered docs (or consulted with pydoc, etc.); it would seem quite rare to me to try to consult a docstring on a class that I'd be patching in the same interactive session (even via the fmt_xdata layer) (but what do I know...).
jklymak commented on Oct 27, 2017
I'd suggest this could go in the toc of doc/users/interactive.rst?
I was going to agree w/ Eric that patching is obscure, but then I realized the only times I've had to chage the format was for moving between x,y and geographic co-ordinates with degrees minutes as dd mm.mmm , and that needs a function.
anntzer commented on Oct 28, 2017
@jklymak Even though it doesn't really help my argument :-) I don't think that's really an issue, its should be exactly the same thing to patch fmt_xdata or format_xdata (or do you have an example?)
jklymak commented on Oct 28, 2017
@anntzer I guess I was specifically thinking about axes.format_coord() which I've overridden to get the lat and lon:
where map is a basemap instance...
But maybe I'm misunderstanding what you mean.
anntzer commented on Oct 28, 2017
Ah, sure, you're talking about overriding another method (and in this specific case it seems a fairly legit use case (yes, I know, you could write a custom formatter and install it as the Axis formatter... but why would you)).
I guess there the argument is that it doesn't make sense to provide specific override points for each methods when you can just override the method itself.
tacaswell commented on Oct 29, 2017
I think we should document setting fmt_xdata and fmt_ydata (rather than document to replace format_xdata) and make the except in format_xdata broader to provide 'lead-underwear' (as one of my colleges at work says). These functions are called on the GUI event loops and should not raise (as that can kill Qt).
Unfortunately format_coord makes the assumption that the x/y values are separable so the only option is to completely replace it. However, that something the project sub-classes should probably just take care of.
tacaswell commented on Oct 29, 2017
and interactive.rst (or something include from it) seems like a good place.
2 remaining items
DOC: document fmt_xdata and fmt_ydata
DOC: document fmt_xdata and fmt_ydata
DOC: document fmt_xdata, fmt_ydata, and fmt_ydata
github-actions commented on Sep 16, 2024
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!
DOC: document fmt_xdata, fmt_ydata, and fmt_ydata