Skip to content

Add ytick label right/left properties in matplotlibrc #10306

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 16 commits into from
Feb 7, 2018
Merged

Add ytick label right/left properties in matplotlibrc #10306

merged 16 commits into from
Feb 7, 2018

Conversation

esvhd
Copy link
Contributor

@esvhd esvhd commented Jan 23, 2018

PR Summary

Add y-axis tick label positions into matplotlibrc for issue #10267

New properties named as:
ytick.labelleft
ytick.labelright

Will also work on a PR for x-axis tick label top/bottom properties for matplotlibrc

which='minor')

self.tick_params(
top=rcParams['xtick.top'] and rcParams['xtick.major.top'],
bottom=rcParams['xtick.bottom'] and rcParams['xtick.major.bottom'],
left=rcParams['ytick.left'] and rcParams['ytick.major.left'],
right=rcParams['ytick.right'] and rcParams['ytick.major.right'],
labelleft=rcParams['ytick.labelleft'] and
rcParams['ytick.major.left'],
Copy link
Member

Choose a reason for hiding this comment

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

You should indent a 8 spaces or so here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually when I indented like the below I got some PEP 8 complains. Was this what you meant?

Alternatively I can have this in a single line but it breaches 80 line margin.

        labelleft=rcParams['ytick.labelleft'] and
            rcParams['ytick.major.left'],

Copy link
Member

Choose a reason for hiding this comment

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

Put the RHS in parentheses and then it should work with indenting...

@jklymak
Copy link
Member

jklymak commented Jan 26, 2018

This at least needs a test. w/o looking at the rest of the code, I don't quite understand how this propagates to the axis drawing.

@jklymak jklymak changed the title Add ytick label right/left properties in matplotlibrc. issue #10267 Add ytick label right/left properties in matplotlibrc Jan 26, 2018
@esvhd
Copy link
Contributor Author

esvhd commented Jan 26, 2018

Essentially I traced down how matplotlibrc was read and how the properties are then set in axes._base__init__(). So I made the change to pick up these two new properties. This did the trick.

All up for testing, any suggestion on how this should be tested? Any example tests you can point me to perhaps?

@jklymak
Copy link
Member

jklymak commented Jan 26, 2018

Tests are in matplotlib/lib/tests. If possible it'd be great if you could set up a testing environment on your local machine before pushing. https://matplotlib.org/devel/testing.html

@esvhd
Copy link
Contributor Author

esvhd commented Jan 27, 2018 via email

@esvhd
Copy link
Contributor Author

esvhd commented Jan 28, 2018

Added a test case test_axes.py::test_axes_tick_params_ylabelside(), similar to test_axes.py::test_axis_set_tick_params_labelsize_labelcolor(), let me know what you think.

I tested the test case manually in a notebook and all asserts worked. However, when I tried to run pytest I got the following error message:

pytest test_axes.py::test_axes_tick_params_ylabelside
INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/_pytest/main.py", line 101, in wrap_session
INTERNALERROR>     config._do_configure()
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/_pytest/config.py", line 921, in _do_configure
INTERNALERROR>     self.hook.pytest_configure.call_historic(kwargs=dict(config=self))
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/pluggy/__init__.py", line 628, in call_historic
INTERNALERROR>     res = self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/pluggy/__init__.py", line 222, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/pluggy/__init__.py", line 216, in <lambda>
INTERNALERROR>     firstresult=hook.spec_opts.get('firstresult'),
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/pluggy/callers.py", line 201, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/pluggy/callers.py", line 76, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/usr/anaconda3/lib/python3.6/site-packages/pluggy/callers.py", line 180, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/home/esvhd/git/esvhd_matplotlib/lib/matplotlib/testing/conftest.py", line 12, in pytest_configure
INTERNALERROR>     matplotlib._init_tests()
INTERNALERROR>   File "/home/esvhd/git/esvhd_matplotlib/lib/matplotlib/__init__.py", line 1448, in _init_tests
INTERNALERROR>     from matplotlib import ft2font
INTERNALERROR> ImportError: cannot import name 'ft2font'

When I ran from matplotlib import ft2font in a python console it actually works fine... Couldn't find much useful info googling this error... Any suggests?

Thx

@jklymak
Copy link
Member

jklymak commented Jan 28, 2018

Are you sure you are using the same environment in each case?

@esvhd
Copy link
Contributor Author

esvhd commented Jan 28, 2018 via email

@jklymak
Copy link
Member

jklymak commented Jan 28, 2018

If you are using conda environments, then fontconfig has to be installed in your test environment. If you aren't using environments you should 😉 https://conda.io/docs/user-guide/tasks/manage-environments.html

@esvhd
Copy link
Contributor Author

esvhd commented Jan 28, 2018 via email

@jklymak
Copy link
Member

jklymak commented Jan 29, 2018

OK, this is almost done. This could use a "whats-new" entry: see doc/users/next_whats_new

Ideally, you would also have an example for it, particulaly one that differentiates it from ax.yaxis.tick_right()... examples/ticks_and_spines I expect. I note that we don't have any examples with tick_right(), so if you could include that too, it'd be fabulous.

Thanks for all your work on this!

@esvhd
Copy link
Contributor Author

esvhd commented Jan 29, 2018 via email

@esvhd
Copy link
Contributor Author

esvhd commented Jan 30, 2018

Added example to show how to set these new properties in rcParams as well as using tick_right() or tick_left(). Created entry for what's new as well. Let me know how this looks. Thanks.

@jklymak
Copy link
Member

jklymak commented Jan 30, 2018

Hmmm. Is this how the example is supposed to look? I'd have expected the ticks and labels to be on the right for the last subplot (ax[2]).

https://5811-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/gallery/ticks_and_spines/tick_label_right.html#sphx-glr-gallery-ticks-and-spines-tick-label-right-py

@afvincent
Copy link
Contributor

afvincent commented Jan 30, 2018

I think that the rcParams might be read at the figure instantiation, i.e. changing them afterwards like in the example will have no effect. (But I may be wrong.)

@QuLogic
Copy link
Member

QuLogic commented Jan 30, 2018

Not Figure, but Axes instantiation (though that's essentially the same time with subplots).

@esvhd
Copy link
Contributor Author

esvhd commented Jan 30, 2018

Fixed the example. Needed to set rcParams before the plt.subplots() is created. Let me know how it looks.

============================================

Example to show how to change two new properties in rcParams to put yaxis tick
lables on either left or right by default.
Copy link
Member

Choose a reason for hiding this comment

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

I think the text should not refer to "new" properties (unless you plan to edit this in a year). Also some formatting changes. (Note I didn't check the comment below for length).

Otherwise looks great!

"""
We can use :rc:`ytick.labelright` (default False) and :rc:`ytick.right` (default False) 
and :rc:`ytick.labelleft` (default True) and :rc:`ytick.left` (default False) to 
control where on the axes ticks and their labels appear.  These properties can also be 
set in the ``.matplotlib/matplotlibrc``.

"""

@esvhd
Copy link
Contributor Author

esvhd commented Jan 31, 2018 via email

@esvhd
Copy link
Contributor Author

esvhd commented Jan 31, 2018

Hope this looks good. Thanks guys!

@afvincent
Copy link
Contributor

About the issue with having the x-ticks and x-tick labels at the top that do not play well with a possible title, I do not think that adding new rcParams is very different from the current situation when the x-ticks and their label are tuned through the proper Axes methods.

I guess that a quick workaround if some core devs are afraid of this issue might be to simply advertise that one can increase the title padding. A genuine first thought that comes to me is something like.

import matplotlib.pyplot as plt

# If a fixed title padding is good enough
#plt.rcParams["axes.titlepad"] = 36  # 24 without x-label

fig, axs = plt.subplots(ncols=2)
for ax, xlbl in zip(axs.flat, ("X-label", "")):
    ax.plot([0, 1])

    ax.set_title("A title")
    ax.set_xlabel(xlbl)
    ax.xaxis.tick_top()
    ax.xaxis.set_label_position('top')

# Dynamic title padding if one prefers
for ax in axs.flat:
    ax.set_title(ax.get_title(), pad=36 if len(ax.get_xlabel()) > 0 else 24)

fig.tight_layout()
plt.show()

@jklymak
Copy link
Member

jklymak commented Feb 2, 2018

Or, you know, #9498 which will do this automatically.

@afvincent
Copy link
Contributor

afvincent commented Feb 2, 2018

I knew that it felt like something that was discussed recently 🐑! Thanks @jklymak for x-referencing.

Edit: I would suggest assuming that #9498 will be merged one day, and thus not bother too much about the top tick labels and x-label overlapping with the title.

@esvhd
Copy link
Contributor Author

esvhd commented Feb 3, 2018

Here we go. Added xtick.labeltop and xtick.labelbottom for matplotlibrc. Also added tests, examples and modified what's new.

Currently this quick solution does not take care of title position. I understood that this is being worked on separately as @jklymak suggested.

Let me know how this looks. Thanks everyone.

@efiring
Copy link
Member

efiring commented Feb 3, 2018

I have a concern about the git history; someone with more git knowledge can comment on whether something should be done about it, and if so, what.

Normally we do not merge master into feature branches. Instead, to keep them up to date, we rebase them on top of the master branch.

@esvhd
Copy link
Contributor Author

esvhd commented Feb 3, 2018

Would be great if someone can shed some light on this, not a git pro here. Thx

@jklymak
Copy link
Member

jklymak commented Feb 3, 2018

I think these are just merge commits and are OK. OTOH these commits would be better if they were rebased into one commit.

Oddly, given how much we ask people to squash things:

https://matplotlib.org/devdocs/devel/gitwash/development_workflow.html

doesn't describe the process, but this is what I do:

https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

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.

Re-approve the new changes.

plt.rcParams['ytick.left'], plt.rcParams['ytick.labelleft'] = False, False


x = np.array([x for x in range(10)])
Copy link
Member

Choose a reason for hiding this comment

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

np.arange(10)

plt.rcParams['xtick.bottom'], plt.rcParams['xtick.labelbottom'] = False, False
plt.rcParams['xtick.top'], plt.rcParams['xtick.labeltop'] = True, True

x = np.array([x for x in range(10)])
Copy link
Member

Choose a reason for hiding this comment

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

np.arange(10)

import matplotlib.pyplot as plt
import numpy as np

plt.rcParams['ytick.right'], plt.rcParams['ytick.labelright'] = True, True
Copy link
Member

Choose a reason for hiding this comment

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

Chained assignment would be a bit simpler:

plt.rcParams['ytick.right'] = plt.rcParams['ytick.labelright'] = True 

ax.tick_params(labelleft=False, labelright=True,
which='minor')
# expects left false, right true
assert(ax.yaxis.majorTicks[0].label1On is False)
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary parentheses.



x = np.array([x for x in range(10)])
x = np.array([x for x in np.range(10)])
Copy link
Member

Choose a reason for hiding this comment

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

No, I mean you can replace the entire line with np.arange(10).

@esvhd
Copy link
Contributor Author

esvhd commented Feb 4, 2018

Added suggested changes from @QuLogic

@esvhd
Copy link
Contributor Author

esvhd commented Feb 4, 2018 via email

@esvhd
Copy link
Contributor Author

esvhd commented Feb 7, 2018

Hi guys, checking in on this, is there any more work need on this PR, particularly from my side?
Thx

@timhoffm timhoffm merged commit 0ec4633 into matplotlib:master Feb 7, 2018
@jklymak
Copy link
Member

jklymak commented Feb 7, 2018

Excellent. Thanks so much @esvhd! Great contribution....

@esvhd
Copy link
Contributor Author

esvhd commented Feb 8, 2018 via email

@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
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.

6 participants