Add minor ticks on and off in Axis #22761

n-aswin · 2022-04-01T09:05:00Z

PR Summary

Simialar to Axes, minor ticks can be turned on or off in Axis using axis.minorticks_on() or axis.minorticks_off().

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

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

oscargus · 2022-04-01T09:53:40Z

doc/users/next_whats_new/axis_minorticks_toggle.rst

+------------------------------
+
+Similar to Axes, minor ticks on Axis can be displayed or removed
+using ``minorticks_on()`` and ``minor_ticks_off()``.


Suggested change

using ``minorticks_on()`` and ``minor_ticks_off()``.

using ``minorticks_on()`` and ``minorticks_off()``.

jklymak · 2022-04-01T11:26:52Z

lib/matplotlib/scale.py

@@ -193,9 +192,9 @@ def set_default_locators_and_formatters(self, axis):
        # update the minor locator for x and y axis based on rcParams
        if (axis.axis_name == 'x' and mpl.rcParams['xtick.minor.visible'] or
                axis.axis_name == 'y' and mpl.rcParams['ytick.minor.visible']):
-            axis.set_minor_locator(AutoMinorLocator())
+            axis.minorticks_off()


This should be "on"? Maybe add a test for this ;-). OTOH This is a change of behaviour, in that it only turns the ticks on, doesn't change then to AutoMinorLocator. I'm not sure this new behaviour is bad, but it might have ramifications for folks.

Yep, my mistake; fixed it.

The definition of minorticks_off() uses AutoMinorLocator unless the scale is log or symlog. Looking at the if condition, wouldn't the logic be better located inside minorticks_on() and minorticks_off()?

I think the (existing) test below covers minorticks_on(), but there are no tests for minorticks_off(). The code passes pytest on test_ticker.py with minorticks_on().

matplotlib/lib/matplotlib/tests/test_ticker.py

Lines 1427 to 1442 in 6c3412b

def test_minorticks_rc():

fig = plt.figure()

def minorticksubplot(xminor, yminor, i):

rc = {'xtick.minor.visible': xminor,

'ytick.minor.visible': yminor}

with plt.rc_context(rc=rc):

ax = fig.add_subplot(2, 2, i)

assert (len(ax.xaxis.get_minor_ticks()) > 0) == xminor

assert (len(ax.yaxis.get_minor_ticks()) > 0) == yminor

minorticksubplot(False, False, 1)

minorticksubplot(True, False, 2)

minorticksubplot(False, True, 3)

minorticksubplot(True, True, 4)

jklymak

Don't we need to change ~.Axes.minortick_on as well?

I think overall this is a welcome addition. Lets try and leave as many breadcrumbs in the docs as possible because folks often get frustrated that the defaults interact difficultly with the more custom locators and tickers.

doc/users/next_whats_new/axis_minorticks_toggle.rst

lib/matplotlib/axis.py

jklymak · 2022-04-02T08:44:31Z

lib/matplotlib/scale.py

@@ -193,9 +192,9 @@ def set_default_locators_and_formatters(self, axis):
        # update the minor locator for x and y axis based on rcParams
        if (axis.axis_name == 'x' and mpl.rcParams['xtick.minor.visible'] or
                axis.axis_name == 'y' and mpl.rcParams['ytick.minor.visible']):
-            axis.set_minor_locator(AutoMinorLocator())
+            axis.minorticks_on()


This line still isn't being hit by the tests? Can we add a test here?

I think the test_minorticks_rc in test_ticker.py only covers linear scale. I can add a test.

n-aswin · 2022-04-02T18:20:22Z

Trying to implement a test which hits all scales; it seems like there are problems. If I understand correctly:

Every scale sets corresponding major and minor locators in the overridden method set_default_locators_and_formatters()
It does this using axis.set_major_locator() and axis.set_minor_locator() which are available through the axis argument to set_default_locators_and_formatters().
Prior to this pull request, toggling minorticks was available through Axes.minorticks_on(). It conditionally checked for 'log' and 'symlog' scales to set appropriate locators while rest of the scales were set to AutoMinorLocator(). This missed proper locators for other items in scales which have specific minor locators (like logit which has LogitLocator(minor=True) or asinh). This pull request only moved the problem into Axis.minorticks_on().
It seems like all the tests in test_ticker.py use the default linear scale and so the line inside FuncScale (mentioned above) was not covered.

Solution 1:

Add every scale name as a condition to Axis.minorticks_on(). Not a clean solution but works; may cause problems when scales added in future are not added as conditions.

Solution 2: (better?)

Add class variables in the ScaleBase class for minor locators and expose them through a ScaleBase.get_minor_locator() method. Setting the variables can be added to set_default_locators_and_formatters(). Axis already has access to scale instances through Axis._scale. Toggling minor ticks in Axis methods can then be done as:

scale = self._scale

# on
minor_locator = scale.get_minor_locator()
self.set_minor_locator(minor_locator)

# off
self.set_minor_locator(mticker.NullLocator())

This should work for scales added in future as long as they have the variables defined. Documentation for Custom Scale can be updated to set the variables.

In both cases, tests for minor ticks can be looped through matplotlib.scale.get_scale_names() (with test function arguments for FuncScale).

I am new to this codebase; let me know if I missed anything.

jklymak · 2022-04-02T19:04:22Z

Thanks, the issues syncing up scales and default locators etc have been a pretty large pain as more scales have been added. I don't know that this PR needs to fix all the woes - if the testing you have in here is as good as the testing you had before, I think its fair enough to get this change in as an improvement, and save sorting out default scale locators and formaters for future work. Thanks a lot for looking into it!

n-aswin · 2022-04-03T08:50:34Z

In that case, I can:

Revert the changes in scale classes so that the locator information is still available there, but add conditional checking of xticks.minor.visible and yticks.minor.visible to remaining built-in scales with documented support (just like how it was implemented before for 'linear' scale).
Add conditional checking in Axis.minorticks_on() for built-in scales (as in Solution 1 mentioned above).
Instead of testing for all scales in matplotlib.scale.get_scale_names(), just test for built-in scales with comments regarding the limited behaviour in the test function.

This way, toggling minor ticks would work for built-in scales with minimum modifications toscale.py. If this sounds good I can push the changes.

oscargus · 2022-04-05T10:46:50Z

Power cycling to restart the tests.

n-aswin · 2022-04-11T04:54:49Z

I have amended the commit seeing that the tests were failing for many scales. Were the rcParams xticks.minor.visible and yticks.minor.visible restricted to only 'linear' and custom scales?

jklymak · 2022-05-18T09:22:53Z

this needs the docs fixed:

/home/circleci/project/doc/users/next_whats_new/axis_minorticks_toggle.rst:4: WARNING: more than one target found for cross-reference 'Axis': mpl_toolkits.mplot3d.axis3d.Axis, matplotlib.axis.Axis
/home/circleci/project/doc/users/next_whats_new/axis_minorticks_toggle.rst:4: WARNING: py:obj reference target not found: Axis.minorticks_on
/home/circleci/project/doc/users/next_whats_new/axis_minorticks_toggle.rst:4: WARNING: py:obj reference target not found: Axis.minorticks_off

n-aswin · 2022-05-26T16:56:47Z

Removed the ambiguity in Axis and added references to doc/api/axis_api.rst.

melissawm · 2023-01-22T21:57:20Z

Hi @n-aswin - this needs a rebase and then I am assuming it is ready for another round of reviews?

If you need help rebasing, here's a nice guide. If you have questions, let us know!

melissawm · 2023-02-08T15:21:37Z

@n-aswin is this ready for review?

n-aswin · 2023-02-09T06:16:49Z

Sorry, forgot to comment after the update. I have rebased the commits; should be OK now.

I had done this quite some time ago. Let me know if this needs changes.

melissawm · 2023-03-09T18:22:58Z

If the PR is ready for review, can you please update it by removing the draft status using the GitHub web interface? Otherwise, it will be skipped over by maintainers, as draft PRs are considered not yet ready to review. Thanks!

jklymak

This looks good to me. Seems a useful addition and maybe we should merge soonish to see what it catches and iterate on fixes

tacaswell · 2023-12-12T23:04:36Z

power cycled to re-run tests against merging to current main

QuLogic · 2023-12-13T02:44:27Z

Looks like it just needs an addition to the type stubs to get the tests passing again.

QuLogic · 2024-03-13T08:23:11Z

I rebased this, and added the missing type hints.

tacaswell · 2024-03-13T20:00:09Z

Thank you for your work @n-aswin and congratulations on your first merged Matplotlib PR 🎉

I hope we hear from you again.

n-aswin · 2024-03-13T20:08:41Z

Thanks!

github-actions bot reviewed Apr 1, 2022

View reviewed changes

oscargus reviewed Apr 1, 2022

View reviewed changes

oscargus added the topic: ticks axis labels label Apr 1, 2022

jklymak reviewed Apr 1, 2022

View reviewed changes

oscargus added the status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. label Apr 1, 2022

jklymak reviewed Apr 2, 2022

View reviewed changes

oscargus closed this Apr 5, 2022

oscargus reopened this Apr 5, 2022

n-aswin force-pushed the axis-minorticks-toggle branch from 66124fc to 3358507 Compare April 11, 2022 04:50

n-aswin force-pushed the axis-minorticks-toggle branch 3 times, most recently from b6283c1 to 20402e8 Compare April 12, 2022 12:26

jklymak marked this pull request as draft May 18, 2022 09:22

jklymak added the status: needs revision label May 18, 2022

github-actions bot added the status: needs rebase label Oct 18, 2022

n-aswin force-pushed the axis-minorticks-toggle branch from 9724a0c to 5c0afab Compare February 2, 2023 05:34

github-actions bot removed the status: needs rebase label Feb 2, 2023

n-aswin marked this pull request as ready for review March 10, 2023 05:48

n-aswin force-pushed the axis-minorticks-toggle branch from 5c0afab to f65b762 Compare March 10, 2023 05:57

rcomer added status: needs review and removed status: needs revision labels Mar 11, 2023

ksunden removed the status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. label Apr 18, 2023

jklymak approved these changes Dec 11, 2023

View reviewed changes

tacaswell closed this Dec 12, 2023

tacaswell reopened this Dec 12, 2023

oscargus added this to the v3.9.0 milestone Dec 13, 2023

dstansby added the New feature label Feb 24, 2024

Add minor ticks on and off in Axis

c6dc3a1

QuLogic force-pushed the axis-minorticks-toggle branch from f65b762 to c6dc3a1 Compare March 13, 2024 08:22

github-actions bot added topic: axes Documentation: API files in lib/ and doc/api labels Mar 13, 2024

tacaswell merged commit 03a7ff0 into matplotlib:main Mar 13, 2024
42 checks passed

n-aswin deleted the axis-minorticks-toggle branch March 13, 2024 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add minor ticks on and off in Axis #22761

Add minor ticks on and off in Axis #22761

n-aswin commented Apr 1, 2022 •

edited

github-actions bot left a comment

oscargus Apr 1, 2022

n-aswin Apr 1, 2022

jklymak Apr 1, 2022

n-aswin Apr 1, 2022

n-aswin Apr 1, 2022 •

edited

jklymak left a comment

jklymak Apr 2, 2022

n-aswin Apr 2, 2022

n-aswin commented Apr 2, 2022 •

edited

jklymak commented Apr 2, 2022

n-aswin commented Apr 3, 2022

oscargus commented Apr 5, 2022

n-aswin commented Apr 11, 2022 •

edited

jklymak commented May 18, 2022

n-aswin commented May 26, 2022

melissawm commented Jan 22, 2023

melissawm commented Feb 8, 2023

n-aswin commented Feb 9, 2023 •

edited

melissawm commented Mar 9, 2023

jklymak left a comment

tacaswell commented Dec 12, 2023

QuLogic commented Dec 13, 2023

QuLogic commented Mar 13, 2024

tacaswell commented Mar 13, 2024

n-aswin commented Mar 13, 2024

	using ``minorticks_on()`` and ``minor_ticks_off()``.
	using ``minorticks_on()`` and ``minorticks_off()``.

	def test_minorticks_rc():
	fig = plt.figure()

	def minorticksubplot(xminor, yminor, i):
	rc = {'xtick.minor.visible': xminor,
	'ytick.minor.visible': yminor}
	with plt.rc_context(rc=rc):
	ax = fig.add_subplot(2, 2, i)

	assert (len(ax.xaxis.get_minor_ticks()) > 0) == xminor
	assert (len(ax.yaxis.get_minor_ticks()) > 0) == yminor

	minorticksubplot(False, False, 1)
	minorticksubplot(True, False, 2)
	minorticksubplot(False, True, 3)
	minorticksubplot(True, True, 4)

Add minor ticks on and off in Axis #22761

Add minor ticks on and off in Axis #22761

Conversation

n-aswin commented Apr 1, 2022 • edited

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

oscargus Apr 1, 2022

Choose a reason for hiding this comment

n-aswin Apr 1, 2022

Choose a reason for hiding this comment

jklymak Apr 1, 2022

Choose a reason for hiding this comment

n-aswin Apr 1, 2022

Choose a reason for hiding this comment

n-aswin Apr 1, 2022 • edited

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

jklymak Apr 2, 2022

Choose a reason for hiding this comment

n-aswin Apr 2, 2022

Choose a reason for hiding this comment

n-aswin commented Apr 2, 2022 • edited

jklymak commented Apr 2, 2022

n-aswin commented Apr 3, 2022

oscargus commented Apr 5, 2022

n-aswin commented Apr 11, 2022 • edited

jklymak commented May 18, 2022

n-aswin commented May 26, 2022

melissawm commented Jan 22, 2023

melissawm commented Feb 8, 2023

n-aswin commented Feb 9, 2023 • edited

melissawm commented Mar 9, 2023

jklymak left a comment

Choose a reason for hiding this comment

tacaswell commented Dec 12, 2023

QuLogic commented Dec 13, 2023

QuLogic commented Mar 13, 2024

tacaswell commented Mar 13, 2024

n-aswin commented Mar 13, 2024

n-aswin commented Apr 1, 2022 •

edited

n-aswin Apr 1, 2022 •

edited

n-aswin commented Apr 2, 2022 •

edited

n-aswin commented Apr 11, 2022 •

edited

n-aswin commented Feb 9, 2023 •

edited