Skip to content
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

Inline iter_ticks into _update_ticks, and use that in mplot3d. #13363

Merged
merged 2 commits into from
Mar 17, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 5, 2019

iter_ticks is only called in one place and the iterator is immediately
converted into a list, so we may as well inline it and create the list
immediately. Also, we can immediately assign the tick locations and
labels, instead of having to carry around the ticks, the locations and
the labels as a tuple.

Reuse that implementation in axis3d (... which also gains support for
minor ticks for free).

xref #13314 (comment).

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

@WeatherGod
Copy link
Member

Just a little bit of a heads-up. We should probably do a sanity check on the image tests, as I think many of them automatically remove tick labels.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 6, 2019

@WeatherGod At least ticks and tick labels on empty 2D and 3D axes look fine.

@@ -1012,6 +1012,7 @@ def _set_artist_props(self, a):
return
a.set_figure(self.figure)

@cbook.deprecated("3.1")
def iter_ticks(self):
Copy link
Member

Choose a reason for hiding this comment

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

This looks great, but I wonder if you want a private _locate_ticks and then call it here, and in the _update_ticks. That way the (deprecated) public function doesn't drift from the private implimentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then (hypothetical, but that's what this discussion is about) changes in _locate_ticks would be reflected in iter_ticks, and you'd need to add additional changelog entries for changes in the behavior of the deprecated method, which seems a bit silly.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 16, 2019

rebased on top of #13314.

iter_ticks is only called in one place and the iterator is immediately
converted into a list, so we may as well inline it and create the list
immediately.  Also, we can immediately assign the tick locations and
labels, instead of having to carry around the ticks, the locations and
the labels as a tuple.

Reuse that implementation in axis3d (... which also gains support for
minor ticks for free).
@anntzer
Copy link
Contributor Author

anntzer commented Mar 13, 2019

Updated another test that suffered from the same issue as #13314 (comment) (minor tick overstrike changes).

``Axis.iter_ticks`` (which only served as a helper to the private
``Axis._update_ticks``) is deprecated.

The signature of the (private) ``Axis._update_ticks`` has been changed to not
Copy link
Member

Choose a reason for hiding this comment

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

axis_artist.AxisArtist._update_ticks() also takes a renderer argument; and it uses it. Not quite clear if we want to keep the APIs similar in both worlds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't even return the same kind of things (one returns a list of ticks, the other returns a bounding box) so I wouldn't worry about that.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I approve.

``Axis.iter_ticks`` (which only served as a helper to the private
``Axis._update_ticks``) is deprecated.

The signature of the (private) ``Axis._update_ticks`` has been changed to not
Copy link
Member

Choose a reason for hiding this comment

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

In that case, I approve.

@timhoffm timhoffm added this to the v3.1.0 milestone Mar 17, 2019
@timhoffm timhoffm merged commit e4cc98b into matplotlib:master Mar 17, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 17, 2019
@anntzer anntzer deleted the ticks branch March 17, 2019 17:46
tacaswell added a commit that referenced this pull request Mar 17, 2019
…363-on-v3.1.x

Backport PR #13363 on branch v3.1.x (Inline iter_ticks into _update_ticks, and use that in mplot3d.)
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 1, 2019
In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic
did not account for the updates from matplotlib#13314.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 9, 2019
In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic
did not account for the updates from matplotlib#13314.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 12, 2019
In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic
did not account for the updates from matplotlib#13314.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants