Skip to content

Move the call to Formatter.set_locs into Formatter.format_ticks. #13323

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

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 30, 2019

All calls to Formatter.set_locs occur before a call to
Formatter.format_ticks (or its predecessor, a list of calls to
Formatter.__call__); it was basically a bandaid around the fact that
all tick locations were needed to properly format an individual tick.

Move the call to set_locs into format_ticks, with the intention to later
deprecate set_locs.

Convert a few more places to use format_ticks.

Milestoning as 3.1 as format_ticks is a new API there so we can still change it before 3.1 is released...

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added this to the v3.1 milestone Jan 30, 2019
@anntzer anntzer force-pushed the formatter.set_locs branch from a8c5557 to d9361a5 Compare January 30, 2019 00:48
@timhoffm
Copy link
Member

timhoffm commented Jan 30, 2019

The cleanup is reasonable. However, I'm stumbling a bit about the changed semantics. format_ticks() does not obviously imply that the internal state is changed (or that it even exists). set_locs() was explicit about that.

Can we get a more clear API? One can have two views on the formatter:

  • simple: The formatter does only format single values.
  • aligned: The format of a tick depends on the properties of all the ticks to be formatted.

fmt.format_ticks(values) feels like the simple formatter, but actually is an aligned formatter. It may be surprising that fmt.format_ticks(values) != [fmt.format_ticks([v])[0] for v in values].

Do we actually need the state? If not one could do something like

def format_ticks(values, context=None):
    """"
    Format *values* in a consistent way.

    The output format of a single value depends on the values in *context*.
    If not given, the context are the values themselves.
    """"
    if context is None:
        context = values

These are just quick thoughts so far. They probably need a bit more consideration.

@jklymak
Copy link
Member

jklymak commented Jan 30, 2019

We already pass the state - thats how ticks = [1, 1.125, 1.25, 1.5] are labeled with the same precision (1.00, 1.125, 1.250, 1.500). Its really makes little sense to not just format all the ticks at once. #10682 is a concrete example of where this is even more useful, and you can think of others. This PR just makes that logic less convoluted.

(`~Formatter.set_locs` followed by repeated `~Formatter.__call__`) have been
updated to use `~Formatter.format_ticks`.

`~Formatter.format_tics` is intended to be overridden by `Formatter` subclasses
Copy link
Member

Choose a reason for hiding this comment

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

tics->ticks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, edited

@timhoffm
Copy link
Member

The issue is that we secretly introduce state.

fmt.set_locs(values)
a = fmt.format_data_short(v)
b = fmt.format_ticks(values)
c = fmt.format_data_short(v)

is explicit and a and c will have the same result.

However, after this PR is in, the code would be

a = fmt.format_data_short(v)
b = fmt.format_ticks(values)
c = fmt.format_data_short(v)

and a and c may be different. That doesn't look like a sane API to me.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Sorry, but I want to block until the API is more clear. I'm quite sure that there is a better way than having set_locs separate from the formatting, but I have to look into this in more detail.

@jklymak
Copy link
Member

jklymak commented Jan 30, 2019

Are there contexts where you would set the locs, but not format the ticks? If not, I'm fine with format_data_short having a dependence on what the major ticks are...

@anntzer
Copy link
Contributor Author

anntzer commented Jan 31, 2019

As @jklymak mentioned, hoping that ticks can be formatted independently from one another is a long-lost battle; there are pretty good reasons why they're interdependent.
I think changing format_data_short (which should really be named format_data_for_cursor, as that's what's it's for) to take the tick locs as argument (and thusly making the formatters stateless) would be a very good idea, but that won't be this PR.
Note that as of now, the calls to set_locs are very, very far apart from the calls to format_data_short (that's effectively in the event handlers for the GUIs...), which makes the statefulness quite hidden too unless you already know about set_locs.

@anntzer anntzer force-pushed the formatter.set_locs branch from d9361a5 to 1e54e65 Compare January 31, 2019 00:52
@timhoffm
Copy link
Member

I agree that the required tick format must depends on all ticks.

I think changing format_data_short (which should really be named format_data_for_cursor, as that's what's it's for) to take the tick locs as argument (and thusly making the formatters stateless) would be a very good idea, but that won't be this PR.

If we get this to work, I'm fine with the present PR, because then the hidden state magic will be gone. I want to look into this before dismissing my change request.

All calls to Formatter.set_locs occur before a call to
Formatter.format_ticks (or its predecessor, a list of calls to
`Formatter.__call__`); it was basically a bandaid around the fact that
all tick locations were needed to properly format an individual tick.

Move the call to set_locs into format_ticks, with the intention to later
deprecate set_locs.

Convert a few more places to use format_ticks.
@anntzer anntzer force-pushed the formatter.set_locs branch from 1e54e65 to da55037 Compare January 31, 2019 10:14
@anntzer
Copy link
Contributor Author

anntzer commented Jan 31, 2019

Just to clarify; are you saying you want the other PR (which doesn't exist right now, we're just discussing design) to be made before unblocking this one?

@timhoffm
Copy link
Member

Not necessarily. I want to be sure that it can be done and I would like to have it at least soon after this one.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 31, 2019

Actually, the easiest is probably just to lookup the tick locations on the axis instance rather than passing it in as argument (otherwise that's what you'll have to do at the call site anyways...), so something like

    def format_data_short(self, value):  # May or may not go through a rename deprecation cycle.
    # or def format_data_short(self, value, ticklocs=None):
        """
        Return a short string version of the tick value.

        Defaults to the position-independent long value.
        """
        # Actually, needs to know whether we are the major or minor formatter,
        # but that's just a matter of `self is self.major.formatter`.
        self.set_locs(self.axis.get_majorticklocs())
        # or
        self.set_locs(ticklocs if ticklocs is not None
                      else self.axis.get_majorticklocs())
        return self.format_data(value)

then slowly move through the different formatter classes to make them independent of set_locs.

@timhoffm
Copy link
Member

timhoffm commented Feb 1, 2019

Hm, the dependence on the axis is another hidden state, and may actually cause incorrect behavior:

formatter = FuncFormatter(lambda x, pos: '_%s_' % x)
ax = plt.gca()
ax.xaxis.set_major_formatter(formatter)
ax.yaxis.set_major_formatter(formatter)

ax.xaxis.get_major_formatter().axis == ax.xaxis   # False!

I wouldn't want to use the axis in the formatter unless really necessary.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 1, 2019

Well, again locators and formatters have known their axis since forever; I guess it would be reasonable to make set_axis raise an error if the axis has already been set (similarly to how artists cannot be reassigned from one axes to another).

@timhoffm
Copy link
Member

timhoffm commented Feb 1, 2019

Well, again locators and formatters have known their axis since forever; I guess it would be reasonable to make set_axis raise an error if the axis has already been set (similarly to how artists cannot be reassigned from one axes to another).

That would make sense.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 1, 2019

Well, except that ThetaLocator is of course doing funky stuff like

class ThetaLocator(mticker.Locator):
    def __init__(self, base):
        self.base = base
        self.axis = self.base.axis = _AxisWrapper(self.base.axis)

    def set_axis(self, axis):
        self.axis = _AxisWrapper(axis)
        self.base.set_axis(self.axis)

Probably can easily be worked around, but will take a bit longer than expected.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 1, 2019

Also, there's a bunch of formatters which actually do not care about the axis (NullFormatter, FixedFormatter, FuncFormatter, FormatStrFormatter, StrMethodFormatter) and these do get used across multiple axes in examples and tests; it also seems like gratuitious breakage to forbid their sharing :/

@jklymak
Copy link
Member

jklymak commented Feb 11, 2019

ping @timhoffm ...

@timhoffm
Copy link
Member

timhoffm commented Feb 14, 2019

Target state:

  1. Tick formats must depend on all ticks.
  2. A formatter is attached to a single axis (and locator). Adding it to mutliple axes must raise an error.
  3. Formatters should not hold a list of tick locs themselves. They should obtain the locs from the axis.

Edit:

Implementation detail:

To maintain the major/minor aspect, formatter should have a flag if they are major or minor. The state should only contain the attibutes axis an is_major.

Alternative B: Pass in the relevant Axis method loc_getter=axis.get_majorticklocs.

Alternative C: Determine is_major = self.axis.major.formatter is self.

I tend to favor alternative C, it's a bit indirect but does not store redundant information.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

Well, I'm not sure what's the best way forward with 2) given my comment just above.

@timhoffm
Copy link
Member

Can we make that optional? The axis is passed in to the formatter, but the formatter can decide not to use axis information, in which case it can be attached to multiple axes. The check for multiple connections would be in the formatter.

Add a class-level flag uses_axis_context

def set_axis(self, axis):
    if not self.uses_axis_context:
        return
    elif: self.axis is not None:
        raise ValueError('XYZFormatter cannot be attached to more than one axis/locator').
    else:
        self.axis = axis

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 14, 2019
@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

OK, that's one possible design.

Alternatively, we could make set_axis (.axis.setter) internally store a list of all axises this formatter has been assigned to, and error out when accessing the .axis attribute (if it has been set to multiple values), which would have the benefit of not requiring third-party formatters to change, and staying automatically correct for all formatter classes.

However that'll likely be a separate PR (the problem of setting a formatter to multiple axises is not new).

Marking this PR as release critical as it changes the semantics of the new format_ticks API and it would be much more of a pain to change this after it has been released.

@timhoffm timhoffm dismissed their stale review February 14, 2019 22:50

Unblocking. I think we're clear now, that the formatter should query the locs from the locator via the axis and not get them passed via set_locs. In that sense, this PR is a reasonable step to internalize getting the locations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: ticks axis labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants