Documentation edit for matshow function #27857
Conversation
|
Yes it's confusing to call a row a "horizontal column". See https://en.m.wikipedia.org/wiki/Row_and_column_vectors |
|
Changed it up to include row and column terminology. Please check. On another note, why are certain checks not passing? As I only changed the documentation right? |
|
The failing tests were unrelated (a new version of pytest broke us). |
|
I see |
|
@Impaler343 I took the liberty of merging my own suggestion for adding spaces, did not seem worth a back-and-forth on, hope you do not mind. |
|
Absolutely fine with it, got a lot to learn! |
There was a problem hiding this comment.
I think there is a reading where the original text was correct (if you pick out a single row, then you move horizontally as you scan through the data in that row), but it also has the opposite reading (as you scan through the rows (at a fixed column) you move horizontally). I do not think either reading is clearly "wrong", so this rewording is an improvement.
lib/matplotlib/pyplot.py
Outdated
| @@ -2438,10 +2438,12 @@ def matshow(A: ArrayLike, fignum: None | int = None, **kwargs) -> AxesImage: | |||
| """ | |||
| Display an array as a matrix in a new figure window. | |||
There was a problem hiding this comment.
| Display an array as a matrix in a new figure window. | |
| Display a 2D array as a matrix in a new figure window. |
lib/matplotlib/pyplot.py
Outdated
| the first index represents the height of the figure | ||
| and the second index represents the width of the figure. |
There was a problem hiding this comment.
Sorry to bikeshed again, but IMHO this is logically incorrect. The index has no direct relation to the height - it's only that first index and height are associated with the same direction (=vertical). Do you have any reservations on my original proposal:
The indexing is ``(row, column)`` so that the first index runs vertically
and the second index runs horizontally in the figure.
|
I would boringly reuse (nearly) the documentation for Axes.matshow:
|
I believe it's important to mention the term "matrix" as 1) it's gives a certain intuitive expectation of the layout even without reading the detailed explanation and 2) it connects to the method name. One could amend your version with that, but I find it then equal to the current version in this PR. So I would just merge without bike shedding further. @anntzer please speak up if you feel strongly about your proposal. |
|
@timhoffm I don't really mind either way. If anything I'd rather try to push users away from matshow()... |
|
I've fixed the squash and rebased on main. Took the liberty to push back to your branch. |
|
Wow, could you tell me what you did? I'm assuming you made another branch and pushed it to this one? |
It would have been easier to just copy the docstring and make a new commit with that, because the change is just in one place. But that would have meant that it's my commit and your attribution would have been lost. So, I've taken the more general route. |
|
Thanks @Impaler343 and congratulations on your first contribution to Matplotlib! 🎉 We hope to see you back. |
PR summary
This changes the documentation of the matshow function, making it clearer as to which dimension corresponds to which number in the array passed to the function. The convention used is standard practice, where the first number(rows) corresponds to the length of the vertical column and the second number(columns) corresponds to the length of the horizontal column.
Closes #27852
PR checklist