Fix: axis, ticks are set to defaults fontsize after ax.clear() #21253
Conversation
There was a problem hiding this comment.
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.
lib/matplotlib/axis.py
Outdated
| @@ -802,6 +802,7 @@ def clear(self): | |||
| """ | |||
|
|
|||
| self.label.set_text('') # self.set_label_text would change isDefault_ | |||
| self.label.set_fontsize(mpl.rcParams['font.size']) | |||
There was a problem hiding this comment.
I suppose we should create a new Text instance here. There are a lot more parameters that could have been set and would need resetting.
There was a problem hiding this comment.
Thank you for your review. I pushed a new commit. A Text instance is created in clear and default values are set.
|
This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details. To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease # assuming you are tracking your branchIf you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you. Thank you for your contributions to Matplotlib and sorry for the inconvenience. |
|
I've popped this to draft until the rebase can be done. @tsumli let us know if you need help with that! |
026417d to
8572dbc
Compare
|
I've done rebase. Do I have other things to do? The problem seems to be fixed. |
|
At the risk of disagreeing with @timhoffm , I have concerns about replacing the text instance. If someone has done fig, ax = plt.subplots()
lab = ax.xaxis.label
ax.cla()
lab.set_text('bob')this would currently work and would not work under this proposal. At a minimum this will need an API change note (in behavior) . I do not think we generically guarantee that any object hanging off of the axes from before If we are going to remove the old |
|
@timhoffm @tacaswell is there more discussion to be had here? Personally I find |
|
Sorry for the late reply. While, I don't think we have given any guarantees that label objects are persistent across clears (ticks labels e.g. obviously aren't), we can jump an extra hoop to maintain that behavior. In that case I suggest to reset all the following text attributes (with the exception of matplotlib/lib/matplotlib/text.py Lines 165 to 184 in fbcfea4 by creating a |
e6a280b to
25a7adb
Compare
|
Sorry for the late commits and replying. _reset_visual_defaults function is created in Text class and used in clear function so that all attributes are initialized if called clear(). |
|
@tsumli Thanks for the update. I’m on holidays right now and cannot properly review for about a week. Will get back to you after that. |
lib/matplotlib/axis.py
Outdated
| @@ -2193,6 +2197,9 @@ class XAxis(Axis): | |||
|
|
|||
| def __init__(self, *args, **kwargs): | |||
| super().__init__(*args, **kwargs) | |||
| self._set_default() | |||
|
|
|||
| def _set_default(self): | |||
There was a problem hiding this comment.
| def _set_default(self): | |
| def _init(self): | |
| """ | |
| Initialize the label and offsetText instance values and | |
| `label_position` / `offset_text_position`. | |
| """ |
I think it's clearer to name this _init because this is really the __init__ logic extracted in a separate method.
lib/matplotlib/text.py
Outdated
| linespacing=None, | ||
| rotation_mode=None, | ||
| **kwargs, | ||
| ): | ||
| self._text = '' |
There was a problem hiding this comment.
Move this line to __init__ (this is just for creating the argument, set_text updates it.
There was a problem hiding this comment.
I also thnik that self.update doesn't really belong in _reset...
There was a problem hiding this comment.
Thank you for your review @oscargus ! but I'm a bit comfused about the first comment.
You mean remove _rest_visual_defaults in Text should be removed and the following lines (ll 200-) are moved in __init__ right?
There was a problem hiding this comment.
Sorry.
I meant that the lines self._text = '' and self.update(**kwargs) should be moved to __init__,
so something like:
self._text = ''
self._reset_visual_defaults(...) # without kwargs
self.update(kwargs)
lib/matplotlib/text.py
Outdated
| @@ -165,6 +165,37 @@ def __init__(self, | |||
| self._x, self._y = x, y | |||
| self._text = '' | |||
| self.set_text(text) | |||
There was a problem hiding this comment.
Sorry, just
self._text = ''
should be here (to initialize the _text attribute).
self.set_text(text) should still go in _reset_visual_defaults. If not, it will not clear the text contents. (If it works, it is a sign of something else being dodgy.)
|
Thanks and congratulations on your first merged PR! Hope to see you around! |
…ntsize after ax.clear()
PR Summary
To solve this issue, not only content but its fontsize will be defaulted value in
clearmethod.PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).