GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — AxisArtist to use standard tick directions and have an option for tick orientation by leejjoon · Pull Request #24553 · matplotlib/matplotlib

Conversation

leejjoon commented Nov 27, 2022

Copy link

Copy Markdown

Contributor

PR Summary

This is a rebased version of PR #19102 that addresses issue #19101. It also add an option to change the orientation of ticks. The ticks in AxisArtist were drawn along the gridlines and may not be perpendicular to spines. This PR introduce an option of tick_orientation that controls this behavior so that it can be normal to spines (see the discussion in #19102).

It will respect "xtick.direction" and "ytick.direction" in rcParams.
The tick.orientation can be one of "parallel", "normal" and "auto". Defaut is "auto".
- 'parallel' - ticks along the grid lines
- 'normal' - ticks normal to axis line.
- 'auto' - 'normal' if tickdir is 'out' else 'parallel'

PR Checklist

Documentation and Tests

Has pytest style unit tests (and pytest passes)
Documentation is sphinx and numpydoc compliant (the docs should build without error).
New plotting related features are documented with examples.

Release Notes

New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

anntzer and others added 6 commits

November 26, 2022 21:51


          Switch AxisArtist to use standard tickdirs.

1561dcd

i.e. defaulting to "out".  Also support "inout".

Would perpendicular work better for "out"?  see e.g.
demo_curvelinear_grid.


          remove the use of _set_external_pad in axisartist

e5790aa


          support axislabel_pad for tickdir==inout

f4ba9af


          add tick_orientation

0b6fcba


          remove unused parasite_cls argument in test_axislines.py

5f5728d


          flake8 fix

8f2dae5

leejjoon force-pushed the aa-tickdir branch from f34a411 to 8f2dae5 Compare

November 27, 2022 08:19

oscargus added the topic: mpl_toolkit label

Nov 28, 2022

oscargus reviewed

Nov 28, 2022

View reviewed changes

Comment thread

lib/mpl_toolkits/axisartist/axis_artist.py


		return tick_orientation

		def set_tick_out(self, b):

oscargus Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe we should deprecate these now? (set/get_tick_out)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

leejjoon Nov 30, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

At what version do we want it to be deprecated?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

oscargus Nov 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd say 3.7.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tacaswell Dec 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this could fit in our normal warn for one release, remove the next pattern, but would defer to you if you wanted to put the new version in for a cycle before we started (or pushed the removal date out further).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

oscargus reviewed

Nov 28, 2022

View reviewed changes

Comment thread

lib/mpl_toolkits/axisartist/axis_artist.py

		# If ticklabels and axislabel are on differenct side, we only
		# conside the padding for the ticks only.

		ticksizes = []

oscargus Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you add a test that uses this path? (And ideally "all" the newly introduced options.)

Edit: one way to at least test the out/in/inout combinations would be to regenerate some of the test images below.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

leejjoon Dec 7, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, I will do.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

oscargus reviewed

Nov 28, 2022

View reviewed changes

Comment thread

lib/mpl_toolkits/axisartist/tests/test_floating_axes.py

		@image_comparison(['curvelinear3.png'], style='default', tol=5)
		def test_curvelinear3():
		# Remove this lines when this test image is regenerated.
		plt.rcParams.update({"xtick.direction": "in", "ytick.direction": "in"})

oscargus Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If I get it correct, this changes the current behavior? Not sure if we should introduce some way to make that transition smoothly, but at least a change note would be in place, I guess.

(Edit: this comment holds for the whole change, not just this line.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tacaswell Dec 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

When we went to v2 we changed ticks from in to out, however because AxisArtist did not follow the rcparams they did not flip when everything else did.

I do not think there is a way to soften this change short of adding another rcparam.

I think we should go forward with this, but make sure there is a very clear API change note.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

leejjoon Dec 7, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I have added a deprecation warning at the level of Tick creation and relevant tests. On the other hand, users won't see the warning unless they manually create a Tick instance. I briefly thought about adding a tickdir keyword argument for Axes initialization, but not very inclined for now. I agree with @tacaswell that it is better to go with it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

oscargus reviewed

Nov 30, 2022

View reviewed changes

Comment thread

lib/mpl_toolkits/axisartist/axis_artist.py

		@@ -62,7 +62,7 @@

oscargus Nov 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The line above (which I cannot mark...) should be updated as well?

Currently:

 ax.axis["bottom"].major_ticks.set_tick_out(True)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

leejjoon added 5 commits

December 7, 2022 17:01


          Deprecate tick_out and add tests for warnings.

bf39e0a


          fix axislabel padding.

95b9e48


          add axis_artist_dir test

d940fb9


          add test for tick_orientation.

d52056b


          minor doc update

a9b049c

github-actions Bot added the status: needs rebase label

Dec 13, 2022

anntzer mentioned this pull request

Jan 7, 2023

Switch AxisArtist to use standard tick directions. #19102

Closed

7 tasks

github-actions Bot commented Feb 9, 2026

Copy link

Copy Markdown

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot added the status: inactive Marked by the “Stale” Github Action label

Feb 9, 2026

QuLogic commented Mar 11, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

#19102 was rebased as #30369, so tick direction should now be available. However, if an option for tick orientation is still desired, this PR can still be used. I have not reviewed the conflicts however, so I don't know how easy that is to untangle.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments

Reviewers

tacaswell

tacaswell left review comments

oscargus

oscargus left review comments

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

status: inactive

Marked by the “Stale” Github Action

status: needs rebase topic: mpl_toolkit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

leejjoon commented Nov 27, 2022

PR Summary

PR Checklist

Uh oh!

oscargus Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscargus Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscargus Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 9, 2026

Uh oh!

QuLogic commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

oscargus Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

oscargus Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

oscargus Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

QuLogic commented Mar 11, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.