matplotlib icon indicating copy to clipboard operation
matplotlib copied to clipboard

Add `Axes.get_tick_params()` method.

Open stefmolin opened this issue 3 years ago • 8 comments

PR Summary

Addresses https://github.com/matplotlib/matplotlib/issues/23603

  • Added Axis.get_tick_params() method, which will return Axis._major_tick_kw or Axis._minor_tick_kw depending on the value of which
  • Added Axes.get_tick_params() method, which will return the result of Axis.get_tick_params() on the axis specified and error out if both is passed in

PR Checklist

Tests and Styling

  • [x] Has pytest style unit tests (and pytest passes).
  • [X] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [ ] New features are documented, with examples if plot related.
  • [ ] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [ ] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [ ] Documentation is sphinx and numpydoc compliant (the docs should build without error).

stefmolin avatar Aug 20 '22 23:08 stefmolin

I believe that this can be one way to go, subject to the caveats in https://github.com/matplotlib/matplotlib/issues/23603#issuecomment-1212572486

I think that there should be a copy of the dictionary returned and that there should be a comment that only those parameters deviating from the default (and/or rcParams?) are returned.

(There should probably also be a version in mplot3d for Axes including a zaxis, but that can be extended in a different PR later.)

oscargus avatar Aug 30 '22 07:08 oscargus

Based on https://github.com/matplotlib/matplotlib/issues/23633 one may also opt for skipping the Axes method and only keep the Axis method. This would avoid any mplot3d issues and keep the API a bit cleaner. (But there is also the discoverability drawback of the Axis API.)

oscargus avatar Aug 30 '22 07:08 oscargus

@stefmolin sorry for letting this slip.

I'm tempted to only add Axis.get_tick_params() for now.

As opposed to the setter, which can set both Axis, and thus can save code, the getter has to resolve to exactly one axis and thus Axes.get_tick_params() is just a 1:1 rewriting of the axis access. We can always add the Axes method later if we consider it reasonable, but adding it now and taking away later is problematic. So, let's be defensive to start with.

We should cross-ref in the docs of Axes.tick_params() and Axis.tick_params() which should reduce the discoverability issue a bit.

A very central part of this, is communicating the meaning of these parameters: They are the default values, i.e. if new ticks are created (e.g. by panning or zooming) these values are used for the new ticks. This is in contrast with the actual tick values (the properties of the current tick objects), which can in principle be overwritten by the user. I think we don't have very clear communication on that right now (in particular also for the setter case tick_params() vs. for tick in ...get_major_ticks(): tick.set_*()), and it would be good to improve that, but maybe that's going a bit far for this PR and "Get the appearance of ticks, tick labels, and gridlines." is a good enough description for a start.

timhoffm avatar Aug 30 '22 11:08 timhoffm

I'm tempted to only add Axis.get_tick_params() for now.

As opposed to the setter, which can set both Axis, and thus can save code, the getter has to resolve to exactly one axis and thus Axes.get_tick_params() is just a 1:1 rewriting of the axis access. We can always add the Axes method later if we consider it reasonable, but adding it now and taking away later is problematic. So, let's be defensive to start with.

Works for me; I removed the extra method.

We should cross-ref in the docs of Axes.tick_params() and Axis.tick_params() which should reduce the discoverability issue a bit.

A very central part of this, is communicating the meaning of these parameters: They are the default values, i.e. if new ticks are created (e.g. by panning or zooming) these values are used for the new ticks. This is in contrast with the actual tick values (the properties of the current tick objects), which can in principle be overwritten by the user.

I updated the docstrings for this – let me know what you think.

I think we don't have very clear communication on that right now (in particular also for the setter case tick_params() vs. for tick in ...get_major_ticks(): tick.set_*()), and it would be good to improve that, but maybe that's going a bit far for this PR and "Get the appearance of ticks, tick labels, and gridlines." is a good enough description for a start.

Not sure what you mean by this. Are you referring to updating a docstring or code?

stefmolin avatar Sep 05 '22 22:09 stefmolin

I think we don't have very clear communication on that right now (in particular also for the setter case tick_params() vs. for tick in ...get_major_ticks(): tick.set_*()), and it would be good to improve that, but maybe that's going a bit far for this PR and "Get the appearance of ticks, tick labels, and gridlines." is a good enough description for a start.

Not sure what you mean by this. Are you referring to updating a docstring or code?

The larger point is that we are not clear in distinguishing between the concept of a Axis-wide default tick style (stored in Axis._major/minor_tick_kw) and the styles of the actual ticks (stored in the individual tick instances). While it's debatable whether such a distinction is desirable, this is how our current implementation works and it shines through in parts of the API, which sometimes causes confusion to the users.

I'm not sure whether documentation or code is the the right way forward. With documentation, we can explain everything in detail. OTOH we could also try to move the user away from interacting with or thinking about single ticks (#23372), which might make a detailed explanation of the topic unnecessary. This would be part code part documentation.

On a side note, the latter would also be in line with ideas for internal refactoring that should increase performance (https://github.com/matplotlib/matplotlib/issues/5665#issuecomment-875155264).

timhoffm avatar Sep 07 '22 08:09 timhoffm

One additional complication I just noted is that the internal names in _major/minor_tick_kw do not match the tick_params() keywords:

https://github.com/matplotlib/matplotlib/blob/3b79f63980c767fabc59ec5ba267818d8b4b5671/lib/matplotlib/axis.py#L948

We have to either translate back for get_tick_params() or clearly document that you get something else :frowning:

I'm sorry this is a bit of a mess.

timhoffm avatar Sep 07 '22 23:09 timhoffm

We have to either translate back for get_tick_params() or clearly document that you get something else 😦

@timhoffm – Is there a preference? I can see these being confusing if you get them back:

https://github.com/matplotlib/matplotlib/blob/3b79f63980c767fabc59ec5ba267818d8b4b5671/lib/matplotlib/axis.py#L1009-L1016

The rest are probably fine. What do you think?

stefmolin avatar Sep 10 '22 19:09 stefmolin

Uh, this is a difficult decision. I'm tempted to translate back. I think the docstring https://matplotlib.org/devdocs/api/_as_gen/matplotlib.axes.Axes.tick_params.html#matplotlib.axes.Axes.tick_params is the only user-visible place where we document what can go in (we currently accept more, but that somewhat coincidence). In that sense, get_tick_params should return the names as given there.

This back-and forth converting is a bit annoying internally, but I think it is the most logical and consistent interface we can offer here.

timhoffm avatar Sep 13 '22 21:09 timhoffm

I added a reverse option to the _translate_tick_params() method and updated it to no longer alter the input dictionary. I'm using this in the forward and reverse directions in the test, so we can make sure both directions work going forward.

stefmolin avatar Oct 22 '22 20:10 stefmolin

@timhoffm - Any ideas what is going on for the docs per this comment above?

Some additional questions on documentation for this PR:

  1. Do we need an example to the docs for this?
  2. Should I add get_tick_params() to the What's New section?
  3. Since _translate_tick_params() no longer alters the keyword dict, should we add it to the API changes? Or does this not matter since it is not for external use?

stefmolin avatar Nov 05 '22 17:11 stefmolin

@timhoffm - Any ideas what is going on for the docs per this comment above?

Please add an entry to doc/api/axis_api.rst

  1. Do we need an example to the docs for this?

You can try and add an Example section to the docstring. I would not make an addition to the Example Gallery.

I hope this is correct, because it'S only from memory - please ignore if I'm wrong: It may be a bit confusing because AFAIR there are a few entries in _major_tick_kw by default. But we're stating that this only contains deviations from defaults (having "deviations from defaults" by default is a bit awkward). OTOH it may make sense to spell this out.

  1. Should I add get_tick_params() to the What's New section?

Yes, please.

  1. Since _translate_tick_params() no longer alters the keyword dict, should we add it to the API changes? Or does this not matter since it is not for external use?

No API change note, because our public API has not changed.

timhoffm avatar Nov 05 '22 17:11 timhoffm

Just close the PR. Open PRs anticipate a follow up action on the PR, even if that action is only "decide what to do".

timhoffm avatar Nov 05 '22 19:11 timhoffm

Just close the PR. Open PRs anticipate a follow up action on the PR, even if that action is only "decide what to do".

@timhoffm What do you mean by this? Was this meant for another PR?

stefmolin avatar Nov 05 '22 19:11 stefmolin

I'm 98% sure @timhoffm meant this for #23787

jklymak avatar Nov 05 '22 21:11 jklymak

Yes, sorry! :sheep:

timhoffm avatar Nov 05 '22 21:11 timhoffm

I hope this is correct, because it'S only from memory - please ignore if I'm wrong: It may be a bit confusing because AFAIR there are a few entries in _major_tick_kw by default. But we're stating that this only contains deviations from defaults (having "deviations from defaults" by default is a bit awkward). OTOH it may make sense to spell this out.

Yeah, there are a few entries in there by default. I agree the phrasing is a little confusing. I modified the docstrings to indicate that we are returning the appearance of any new elements that will be added without calling out defaults – hopefully, that makes it easier to understand.

I also added the example to the docstring and what's new entry.

stefmolin avatar Nov 06 '22 01:11 stefmolin

Should I also add the .. versionadded:: directive per the latest version of the PR template? Would this be for 3.7?

stefmolin avatar Nov 12 '22 02:11 stefmolin

@jklymak - I added the .. versionadded:: directive and tweaked the docstrings and example per your comments.

stefmolin avatar Nov 13 '22 01:11 stefmolin

Thank you @stefmolin !

tacaswell avatar Nov 23 '22 21:11 tacaswell