Skip to content

Add Axes.get_tick_params() method. #23692

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 14 commits into from
Nov 23, 2022
Merged

Conversation

stefmolin
Copy link
Contributor

@stefmolin stefmolin commented Aug 20, 2022

PR Summary

Addresses #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

  • Has pytest style unit tests (and pytest passes).
  • 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).
  • [N/A] 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).

@oscargus
Copy link
Member

I believe that this can be one way to go, subject to the caveats in #23603 (comment)

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
Copy link
Member

Based on #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 oscargus added the status: needs comment/discussion needs consensus on next step label Aug 30, 2022
@timhoffm
Copy link
Member

@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.

@stefmolin
Copy link
Contributor Author

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 stefmolin changed the title [WIP] Add Axes.get_tick_params() method. Add Axes.get_tick_params() method. Sep 5, 2022
@timhoffm
Copy link
Member

timhoffm commented Sep 7, 2022

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 (#5665 (comment)).

@timhoffm
Copy link
Member

timhoffm commented Sep 7, 2022

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

kwtrans = self._translate_tick_params(kwargs)

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

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

@stefmolin
Copy link
Contributor Author

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:

'left': 'tick1On',
'bottom': 'tick1On',
'right': 'tick2On',
'top': 'tick2On',
'labelleft': 'label1On',
'labelbottom': 'label1On',
'labelright': 'label2On',
'labeltop': 'label2On',

The rest are probably fine. What do you think?

@timhoffm
Copy link
Member

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.

@stefmolin
Copy link
Contributor Author

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
Copy link
Contributor Author

@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?

@timhoffm
Copy link
Member

timhoffm commented Nov 5, 2022

@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
Copy link
Member

timhoffm commented Nov 5, 2022

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

@stefmolin
Copy link
Contributor Author

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?

@jklymak
Copy link
Member

jklymak commented Nov 5, 2022

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

@timhoffm
Copy link
Member

timhoffm commented Nov 5, 2022

Yes, sorry! 🐑

@stefmolin
Copy link
Contributor Author

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.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Just a few documentation suggestions. I didn't read the history, but the docs refer to a mysterious "default style" which to me are the rcParam values, not the values currently on the axis. I think you are trying to differentiate between manual ticks and those styles automatically, but I think the note gets that across adequately. People don't often much with individual ticks, so I'd argue not to muddy the waters of the main docstrings with a rare case.

@stefmolin
Copy link
Contributor Author

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

@stefmolin
Copy link
Contributor Author

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

@tacaswell tacaswell merged commit 7c0d03a into matplotlib:main Nov 23, 2022
@tacaswell
Copy link
Member

Thank you @stefmolin !

@stefmolin stefmolin deleted the get-tick-params branch November 23, 2022 21:50
@QuLogic QuLogic added this to the v3.7.0 milestone Nov 23, 2022
@@ -988,19 +1051,27 @@ def _translate_tick_params(kw):
'labelright': 'label2On',
'labeltop': 'label2On',
Copy link

Choose a reason for hiding this comment

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

labelleft & labelbottom have the same axis key, the translation will therefore always return labelleft and labelright, also for the x-axis. This is confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Please open a new issue with the details of what you are seeing.

Choose a reason for hiding this comment

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

Ok, done, see #28772

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs comment/discussion needs consensus on next step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants