axis3d.py : new inheritance and overriden methods for Yaxis class #21927
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.
| self.figure.dpi_scale_trans) | ||
| length = ends.height * 72 | ||
| # The spacing must be the same as in the XAxis method (3). | ||
| size = self._get_tick_label_size('y') * 3 |
There was a problem hiding this comment.
AFAICS, using 'y' is the only difference to before. In that case, shouldn't we fix the original Axis.get_tickspace to use self._get_tick_label_size(self.axis_name) (and define axis_name on the 3d Axis classes)?
There was a problem hiding this comment.
Well, the only difference to before is in fact not the use of 'y' but the multiplication by 3. We can see in the axis.py file that the method get_tick_space in the XAxis class has the line size = self._get_tick_label_size('x') * 3, whereas in the YAxis class the same method has the line size = self._get_tick_label_size('y') * 2
The reason for this change in coefficient is the aspect of a 2D plot : it requires less space to write labels like this :
label (Y labels)
label
label
Than like this :
label label label (X labels)
However, in 3D plots the Y axis is like another X axis : hence the overriding of the method, even though it's just for one digit.
There was a problem hiding this comment.
Thanks for the clarification. It seems we should turn the factor into a class variable then.
There was a problem hiding this comment.
You're probably right, it would be more convenient. Oh and I also had a question : would you know why thecodecov/progect/tests omitted the src file in this PR ? I'm new to github and matplotlib, I don't really understand how all of it works.
There was a problem hiding this comment.
I have to admit, that I don't understand Codecov either. It sometimes appears to do weird things.
|
I'm going to close this PR because the issue it was addressing has subsequently been fixed by #21442 (which is likely the source of the need to rebase). As was identified in the in-line discussion, in 3D we want to use the x-axis spacing for the y-axis as well as the x-axis. However, we are currently get that by inheriting all of the @coding-krtek Thank you for your work on this. I'm sorry this languished in review and did not get merged but I hope will hear from you again. |
PR Summary
This pull request is a solution to this issue that seems to work. This issue is about the axes in a 3d plot.
In mplot3d/axis3d.py, because of a bad inheritance, as @QuLogic pointed out, methods like
invert.yaxis()are affecting the X axis and not the Y axis.He proposed a solution that I have copied here (changing inheritance in
the
[ X,Y,Z]Axisclasses).But because of this change the ticks of the Y axis started to behave as if it was a 2D plot : this is what I tried to solve.
My contribution boils down to overriding two methods in the YAxis class in mplot3d/axis3d.py :
get_tick_spacemethod (the spacing must be of 3 as in the axis.XAxis method, and not just 2)clearmethod (this method sets the padding of the Y ticks as in a 2D plot, and is called "at the last second". I found nothing better than fixing the padding right after this method is called)PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).