Add rcParams to set ndivs minor ticks on axes separately #14355

Adioosin · 2019-05-28T18:03:05Z

PR Summary

added the key in rcParams for xtick.minor.default. didn't change the number of parameters of Autominorticker. if the rcparams is set and the parameter value is none then use the parameter value for minor tick or else use the n value provided. for #14233

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

Adioosin · 2019-05-28T18:10:01Z

@timhoffm is the new parameter userc in Autominorticker.init required can't it be done like this i.e. if n=None then use rcParams else just read the n value.

tacaswell · 2019-05-28T18:14:16Z

Welcome to Matplotlib!

Could we change the name to something more descriptive of what it is (like 'xxx.xxx.ndivs`).

It looks like this adds both an xtick and ytick version of the rcparam, but only uses the xtick flavor the code.

Could you add a test which changes the rcparam and then verifies that the number of divisions actually changes. Remember tests are less about proving what you did works now (although, that is a side effect) it is about ensuring that future changes don't break it!

anntzer · 2019-05-28T20:03:31Z

lib/matplotlib/ticker.py

            else:
-                ndivs = 4
+                ndivs = rcParams['xtick.minor.default']


It may make more sense to have the default rcParam value be None, which would mean "4-or-5", and otherwise just use a numeric value as is.

timhoffm · 2019-05-28T20:11:20Z

lib/matplotlib/ticker.py

@@ -2658,9 +2658,9 @@ def __call__(self):
            majorstep_no_exponent = 10 ** (np.log10(majorstep) % 1)

            if np.isclose(majorstep_no_exponent, [1.0, 2.5, 5.0, 10.0]).any():
-                ndivs = 5
+                ndivs = rcParams['xtick.minor.default'] + 1


I don't think this generalization is the way to go. This distinction is specifically designed to give reasonable intervals for certain interval steps, e.g.
for a majorstep of 10, you get 5 intervals, i.e. ticks at [0, 2, 4, 5, 6, 10]. Similar for 2.5 you would get ticks at [0, 0.5, 1, 1.5, 2, 2.5]. This does not translate well to other numbers of divisions.

If the user sets an explicit number, he should exactly get that. Additionally, we need some 'auto' value which triggers the current 4/5 mechanism.

That's basically my suggestion above -- I guess "auto" is a nicer spelling than "None", though, I like it.

Also, it may make sense to do the check on the rcParam in the constructor instead (whose docstring also needs to be updated).

Case 1:

Autominorticker(n=none) and rcParams[xtick.minor.ndivs] = none then the default n = 4 or 5 case.

Autominorticker(n=none) and rcParams[xtick.minor.ndivs] = some value then set n = some value.

Autominorticker(n=somevalue) and rcParams[xtick.minor.ndivs] = some value or Autominorticker(n=somevalue) and rcParams[xtick.minor.ndivs] = none then set n = value in argument of Autominorticker.

Allowed value of rcParams = {none, int}

Case 2:

Autominorticker(n=none, auto=True) then use n = rcParams[xtick.minor.ndivs] .

Autominorticker(n=none, auto=False) then use 'n = 4 or 5'.

Autominorticker(n=somevalue, auto=False) then use 'n = somevalue'.

Autominorticker(n=somevalue, auto=True) then also 'n = somevalue'.

Allowed value of rcParams = {int}

which case will be better for the feature @timhoffm @anntzer .

Thanks for working out the possibilities.
I would strongly prefer 1) (no need to add a new parameter), let's see what @timhoffm says.

This is one functionality, and this should be configured by a single parameter. Having multiple parameters with overlapping meaning gets us into trouble regularly.

I would slightly prefer an additional 'auto' value:

Allowed value of rcParams: {'auto', int}

Allowed value of Autominorticker(n): {'auto', int, None}

with:

None: use rcParams

int: use explicitly that many ticks

'auto': automatically choose a good number of ticks (the current 4/5 mechanism)

This has the advantage of a clear distinction between "fall back to rc" and "find a reasonable value", which are semantically mixed in the proposal 1) above.

I need a little help, how can i know on which axis the AutoMinorLocator() is called
Thank you

You can look up self.axis.__name__.

timhoffm · 2019-05-28T20:16:07Z

lib/matplotlib/rcsetup.py

@@ -1310,6 +1310,7 @@ def _validate_linestyle(ls):
    'xtick.minor.bottom':    [True, validate_bool],    # draw x axis bottom minor ticks
    'xtick.major.top':   [True, validate_bool],  # draw x axis top major ticks
    'xtick.major.bottom':    [True, validate_bool],    # draw x axis bottom major ticks
+    'xtick.minor.default':  [4,validate_int],   #set default value for x axis minor ticks


Suggested change

'xtick.minor.default': [4,validate_int], #set default value for x axis minor ticks

'xtick.minor.default': [4, validate_int], #set default value for x axis minor ticks

Please adhere to the pep8 style conventions. We enforce these to keep the codebase clean and readable. If you check the CI runs at the bottom of the PR #14355, you'll notice that the flake8 run in travis complains on some codestyle issues.

Adioosin · 2019-05-29T09:17:16Z

@anntzer i had a doubt. i forked the copy of matplotlib then worked on it(added this feature) now if i want to test the feature that i added how do i test it. If I import the matplotlib package it just takes the one that is installed in the anaconda site-packages.

anntzer · 2019-05-29T09:28:52Z

https://matplotlib.org/devel/testing.html, https://matplotlib.org/devel/contributing.html#installing-for-devs.

dstansby · 2019-06-02T17:41:32Z

@anntzer has posted the correctly links for installing a 'editable' local copy, but I just wanted to add feel free to ask any questions here if you run into any problems with those instructions.

Adioosin · 2019-06-04T15:19:21Z

@anntzer @dstansby I am having problem with the local copy installation from source.
The Error message is LINK : fatal error LNK1181: cannot open input file 'png.lib' i can't find png.lib anywhere so that i can explicitly set its location. i found libpng.lib instead is that the same thing.

anntzer · 2019-06-04T15:33:26Z

Yes (#13084, #13077).

Adioosin · 2019-06-05T05:47:51Z

Do we need separate default ndivs for both the axis in rcParams or a single one.
If single then Autominorlocator's locator will decide the axis to which minor ticker will be shown.
If separate then the rcParams's xxx.minor.visible = True will take corresponding value when called. @anntzer @timhoffm @dstansby

When using self.axis.__name__ to get the locators axis it returns 'NoneType' object has no attribute '__name__'

anntzer · 2019-06-05T07:42:52Z

lib/matplotlib/ticker.py

@@ -2634,7 +2634,15 @@ def __init__(self, n=None):

        If *n* is omitted or None, it will be set to 5 or 4.
        """
-        self.ndivs = n
+        if n == None:
+            if Locator.axis.__name__ == 'xaxis':


At that point self.axis is not set yet, but it should be in __call__.

🙁 That means we can only to the default resolution in __call__ and not in __init__. That's a bit unusual, but there's probably no way around; and hopefully nobody will stumble over it.

Adioosin · 2019-06-05T12:45:05Z

I had a doubt how to know why codecov/project/tests is failing. Is it because i didn't add any test for the feature? if so then the test need to be added to matplotlib.tests.test_ticker?

timhoffm · 2019-06-05T21:25:18Z

lib/matplotlib/ticker.py

@@ -2634,10 +2634,21 @@ def __init__(self, n=None):

        If *n* is omitted or None, it will be set to 5 or 4.
        """
-        self.ndivs = n
+        if n is None:


This whole block is equivalent to self.ndivs = n. Why is it here? Did you want to have a case where that's not true?

Sorry my bad, i left that chuck while trying to solve the problem via __init__ method. Will correct it back.

timhoffm · 2019-06-05T21:35:01Z

I had a doubt how to know why codecov/project/tests is failing. Is it because i didn't add any test for the feature? if so then the test need to be added to matplotlib.tests.test_ticker?

I also have a hard time of understanding the codecov metrics, but probably you are right about missing tests, in the sense that you added a bit of code that is not excercised by any tests. Tests are always welcome, but since you're a first time-contributor and this is a relatively clear change I wouldn't strictly require it, if you feel it's too much for now. 😄

Adioosin · 2019-06-06T02:00:02Z

I had a doubt how to know why codecov/project/tests is failing. Is it because i didn't add any test for the feature? if so then the test need to be added to matplotlib.tests.test_ticker?

I also have a hard time of understanding the codecov metrics, but probably you are right about missing tests, in the sense that you added a bit of code that is not excercised by any tests. Tests are always welcome, but since you're a first time-contributor and this is a relatively clear change I wouldn't strictly require it, if you feel it's too much for now. 😄

I would really like to make a full contribution and if the codecov is not passed it can't be merged right? @timhoffm

Adioosin · 2019-06-22T09:11:31Z

I will write the test if you want it for merging. will it be a image test or just simple test in test/test_ticker.py @timhoffm .

timhoffm · 2019-06-22T23:26:44Z

We try to only use the @image_comparison decorator only when it it's really neccessary because the reference images bolow up the size of the git repoisitory significantly. In this case, a @check_figures_equal should be preferred.

tacaswell · 2023-05-24T20:16:47Z

Thank you for your work on this @Adioosin , however we have merged #18715 which resolves the same issue.

adding rcparams

f76508d

tacaswell added this to the v3.2.0 milestone May 28, 2019

anntzer reviewed May 28, 2019

View reviewed changes

timhoffm reviewed May 28, 2019

View reviewed changes

Adioosin changed the title ~~adding rcparams default~~ Add rcParams to set ndivs minor ticks on axes separately May 29, 2019

Aditya Sinha A added 2 commits June 1, 2019 00:31

Autominorticker feature added

ba2c9c5

Autominorticker feature added

428eb89

anntzer mentioned this pull request Jun 4, 2019

When linking against libpng/zlib on Windows, use upstream lib names. #13084

Merged

6 tasks

anntzer reviewed Jun 5, 2019

View reviewed changes

Adioosin added 2 commits June 5, 2019 14:06

added auto feature to ndivs for both the axes

a14d680

travis changes

9078458

travis changes

104dc40

timhoffm reviewed Jun 5, 2019

View reviewed changes

ticker.py updated

b5038d5

QuLogic added the status: needs tests label Jul 25, 2019

timhoffm added the status: needs rebase label Aug 16, 2019

timhoffm removed this from the v3.2.0 milestone Aug 16, 2019

timhoffm added this to the v3.3.0 milestone Aug 16, 2019

QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020

jklymak added the status: orphaned PR label Sep 10, 2020

jklymak marked this pull request as draft September 10, 2020 15:27

QuLogic modified the milestones: v3.4.0, unassigned Jan 21, 2021

story645 modified the milestones: unassigned, needs sorting Oct 6, 2022

tacaswell closed this May 24, 2023

QuLogic added the status: superseded label Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rcParams to set ndivs minor ticks on axes separately #14355

Add rcParams to set ndivs minor ticks on axes separately #14355

Adioosin commented May 28, 2019

Adioosin commented May 28, 2019

tacaswell commented May 28, 2019

anntzer May 28, 2019

timhoffm May 28, 2019

anntzer May 28, 2019 •

edited

Loading

Adioosin May 29, 2019

anntzer May 29, 2019

timhoffm May 31, 2019

Adioosin May 31, 2019 •

edited

Loading

anntzer May 31, 2019

timhoffm May 28, 2019

Adioosin commented May 29, 2019

anntzer commented May 29, 2019

dstansby commented Jun 2, 2019

Adioosin commented Jun 4, 2019

anntzer commented Jun 4, 2019

Adioosin commented Jun 5, 2019 •

edited

Loading

anntzer Jun 5, 2019

timhoffm Jun 5, 2019

Adioosin commented Jun 5, 2019

timhoffm Jun 5, 2019

Adioosin Jun 6, 2019 •

edited

Loading

timhoffm commented Jun 5, 2019

Adioosin commented Jun 6, 2019 •

edited

Loading

Adioosin commented Jun 22, 2019

timhoffm commented Jun 22, 2019 •

edited

Loading

tacaswell commented May 24, 2023

	'xtick.minor.default': [4,validate_int], #set default value for x axis minor ticks
	'xtick.minor.default': [4, validate_int], #set default value for x axis minor ticks

Add rcParams to set ndivs minor ticks on axes separately #14355

Add rcParams to set ndivs minor ticks on axes separately #14355

Conversation

Adioosin commented May 28, 2019

PR Summary

PR Checklist

Adioosin commented May 28, 2019

tacaswell commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adioosin May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adioosin commented May 29, 2019

anntzer commented May 29, 2019

dstansby commented Jun 2, 2019

Adioosin commented Jun 4, 2019

anntzer commented Jun 4, 2019

Adioosin commented Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adioosin commented Jun 5, 2019

Choose a reason for hiding this comment

Adioosin Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

timhoffm commented Jun 5, 2019

Adioosin commented Jun 6, 2019 • edited Loading

Adioosin commented Jun 22, 2019

timhoffm commented Jun 22, 2019 • edited Loading

tacaswell commented May 24, 2023

anntzer May 28, 2019 •

edited

Loading

Adioosin May 31, 2019 •

edited

Loading

Adioosin commented Jun 5, 2019 •

edited

Loading

Adioosin Jun 6, 2019 •

edited

Loading

Adioosin commented Jun 6, 2019 •

edited

Loading

timhoffm commented Jun 22, 2019 •

edited

Loading