Skip to content

Cleaning up the Formatter API #5787

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

Open
anntzer opened this issue Jan 2, 2016 · 9 comments
Open

Cleaning up the Formatter API #5787

anntzer opened this issue Jan 2, 2016 · 9 comments
Labels
keep Items to be ignored by the “Stale” Github Action

Comments

@anntzer
Copy link
Contributor

anntzer commented Jan 2, 2016

Still on my offset-text rewrite spree...

Currently, the formatter API (in charge of picking the strings for the tick labels, axis offset and cursor position in the status bar) is a bit messy: the public methods available are

  • Formatter.__call__(self, val, idx): computes the label texts
  • Formatter.format_data(self, val): not publicly used
  • Formatter.format_data_short(self, val): computes the cursor position text
  • Formatter.get_offset(self): returns the offset text
  • Formatter.set_locs(self, locs): required for computing the labels
  • Formatter.fix_minus(self, s): overriden only by ScalarFormatter to use a unicode minus

FixedFormatter also provides set_offset_string.

The commonly used ScalarFormatter and LogFormatter also provide pprint_val (with different signatures, and not publicly used).

ScalarFormatter also provides a couple of getter/setters (for useOffset, useLocale, scientific, powerlimits); LogFormatter provides base and label_minor as setters for instance variables.

LogFormatterExponent and LogFormatterMathtext are variants of LogFormatter.

EngFormatter also provides format_eng (not publicly used).

There are also a bunch of instance attributes which are public but probably intended as private.

I propose simplifying the API to the following:

  • Formatter.get_label_text(self, val: float, idx: int): computes the label texts
  • Formatter.get_cursor_text(self, val: float): computes the cursor position text
  • Formatter.get_offset_text(self) and Formatter.set_offset_text(self, s: str): get/set the offset text
  • Formatter.set_locs(self, locs: array-like): because we need it
  • Formatter.set_uselocale(self, b: bool): because there's no reason only ScalarFormatter has it

(the new method names can be made to fallback on the old method names with a deprecation warning in the base class)

and drop or make private basically everything else:

  • fix_minus (let's first check if any backend doesn't support unicode)
  • pprint_val/format_eng
  • the various instance attributes which should have been private to start with (keeping: scientific, powerlimits, base, label_minor, using getter/setters)

and fold the LogFormatterExponent and LogFormatterMathText classes into options for the LogFormatter class.

Thoughts?

@tacaswell
Copy link
Member

From 10k feet looks good.

Keep in mind the coming conversion to traitlets which is going to deprecate most of the getter/setter methods (set_locs and set_uselocale both fall in that camp, maybe offset_text as well). It might be better to use names without get for the public methods. Maybe Formatter.for_label and for_cursor

I have a preference for more simpler functions/classes over fewer more complex functions/classes. Having attributes/arguments that affect drawing conditional on the value of some other attribute/argument makes the API more frustrating to use.

@tacaswell tacaswell added this to the proposed next point release (2.1) milestone Jan 2, 2016
@tacaswell
Copy link
Member

Also going back and forth on if this should be lumped in with 2.0 style changes or 2.1. Tagged this as 2.1, but if these changes make the style changes to the offset text easier to do I am in favor of re-implementing the old API in terms of the new, using the new everywhere internally and starting the deprecation cycle on the old in 2.1 (to be removed in 2.3).

@anntzer
Copy link
Contributor Author

anntzer commented Jan 3, 2016

Keep in mind the coming conversion to traitlets which is going to deprecate most of the getter/setter methods (set_locs and set_uselocale both fall in that camp, maybe offset_text as well). It might be better to use names without get for the public methods. Maybe Formatter.for_label and for_cursor

What's the status of the switch to traitlets? (Specifically, is it certain that there will be a switch?) Given that the Formatter class is actually fairly self-contained, would it be OK for me to directly write a traitlets-based API? (I don't want to maintain two compatibility layers.)

I would suggest Formatter.as_label_text and Formatter.as_cursor_text, perhaps.

I have a preference for more simpler functions/classes over fewer more complex functions/classes. Having attributes/arguments that affect drawing conditional on the value of some other attribute/argument makes the API more frustrating to use.

Do you mean that you'd rather keep LogFormatterExponent and LogFormatterMathText out? I personally think that ax.get_xaxis().get_major_formatter().use_mathtext = False would be easier to use than having to import a new class and swap it in with set_major_formatter for that purpose.

@tacaswell
Copy link
Member

We have not hit a major technical issue with traitlets yet. Unless there is
something like a show stopping unresolvable speed regression that will be
one of two main features for 2.1 (the other being the gui classes overhaul).

On Sat, Jan 2, 2016, 20:18 Antony Lee notifications@github.com wrote:

Keep in mind the coming conversion to traitlets which is going to
deprecate most of the getter/setter methods (set_locs and set_uselocale
both fall in that camp, maybe offset_text as well). It might be better to
use names without get for the public methods. Maybe Formatter.for_label and
for_cursor

What's the status of the switch to traitlets? (Specifically, is it certain
that there will be a switch?) Given that the Formatter class is actually
fairly self-contained, would it be OK for me to directly write a
traitlets-based API? (I don't want to maintain two compatibility
layers.)

I would suggest Formatter.as_label_text and Formatter.as_cursor_text,
perhaps.

I have a preference for more simpler functions/classes over fewer more
complex functions/classes. Having attributes/arguments that affect drawing
conditional on the value of some other attribute/argument makes the API
more frustrating to use.

Do you mean that you'd rather keep LogFormatterExponent and
LogFormatterMathText out? I personally think that ax.get_xaxis().get_major_formatter().use_mathtext
= False would be easier to use than having to import a new class and swap
it in with set_major_formatter for that purpose.


Reply to this email directly or view it on GitHub
#5787 (comment)
.

@mdboom
Copy link
Member

mdboom commented Jan 4, 2016

It would be nice to have this on 2.0 (I think), which won't have Traitlets. So, unfortunately, if we agree it should be on 2.0, we will have to do a non-Traitlets implementation now and a Traitlets one later. In our experience with Traitlets so far, though, this won't mean maintaining two code paths, but simply replacing to use Traitlets.

@tacaswell
Copy link
Member

A pattern we used else where is to use the 'old-school' property model to
get both the getter/setter api + property API.

On Mon, Jan 4, 2016 at 10:19 AM Michael Droettboom notifications@github.com
wrote:

It would be nice to have this on 2.0 (I think), which won't have
Traitlets. So, unfortunately, if we agree it should be on 2.0, we will have
to do a non-Traitlets implementation now and a Traitlets one later. In our
experience with Traitlets so far, though, this won't mean maintaining two
code paths, but simply replacing to use Traitlets.


Reply to this email directly or view it on GitHub
#5787 (comment)
.

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Oct 3, 2017
@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 Mar 20, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Mar 20, 2023

This is somewhat outdated, but we could still at least consider deprecating (or rather making private) format_data, which is not actually used anywhere other than as a internal helper, as well as set_locs (per #13323).

@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label Mar 21, 2023
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 Mar 22, 2024
@QuLogic QuLogic added keep Items to be ignored by the “Stale” Github Action and removed status: inactive Marked by the “Stale” Github Action labels Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep Items to be ignored by the “Stale” Github Action
Projects
None yet
Development

No branches or pull requests

4 participants