Skip to content
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

Documentation edit for matshow function #27857

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

Impaler343
Copy link
Contributor

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

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I would like the row, column terminology to be included somehow as that’s how matrices are usually described.

@jklymak
Copy link
Member

jklymak commented Mar 4, 2024

Yes it's confusing to call a row a "horizontal column". See https://en.m.wikipedia.org/wiki/Row_and_column_vectors

@Impaler343
Copy link
Contributor Author

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?

@tacaswell tacaswell added this to the v3.9.0 milestone Mar 4, 2024
lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved
@tacaswell
Copy link
Member

The failing tests were unrelated (a new version of pytest broke us).

@Impaler343
Copy link
Contributor Author

I see

@tacaswell
Copy link
Member

@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.

@Impaler343
Copy link
Contributor Author

Absolutely fine with it, got a lot to learn!

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

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.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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 Show resolved Hide resolved
lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved
Comment on lines 2443 to 2444
the first index represents the height of the figure
and the second index represents the width of the figure.
Copy link
Member

@timhoffm timhoffm Mar 6, 2024

Choose a reason for hiding this comment

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

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.

@anntzer
Copy link
Contributor

anntzer commented Mar 7, 2024

I would boringly reuse (nearly) the documentation for Axes.matshow:

Plot the values of a 2D array as a color-coded image in a new Figure window.

The array is shown as it would be printed, with the first row at the top. The aspect ratio...

@timhoffm
Copy link
Member

timhoffm commented Mar 7, 2024

I would boringly reuse (nearly) the documentation for Axes.matshow:

Plot the values of a 2D array as a color-coded image in a new Figure window.

The array is shown as it would be printed, with the first row at the top. The aspect ratio...

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.

@anntzer
Copy link
Contributor

anntzer commented Mar 7, 2024

@timhoffm I don't really mind either way. If anything I'd rather try to push users away from matshow()...

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved
@timhoffm
Copy link
Member

I've fixed the squash and rebased on main. Took the liberty to push back to your branch.

@Impaler343
Copy link
Contributor Author

Impaler343 commented Mar 14, 2024

Wow, could you tell me what you did? I'm assuming you made another branch and pushed it to this one?

@timhoffm
Copy link
Member

  1. Checked what's on this branch.
  2. Decided to go with cherry-picking instead of interactive rebase given that only a few commits of this branch were relevant.
  3. Created a new branch and cherry-picked the relevant commits. For one of them with only partly relevant content (likely from a merge), I created a new commit instead (while one can cherry-pick partial commits, just copying the one relevant code change and committing that was easier).
  4. Squashed the commits in the new branch.
  5. Created a backup branch for this branch.
  6. Reset --hard this branch on the new branch.
  7. Force pushed.

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.

@timhoffm timhoffm merged commit 48c065e into matplotlib:main Mar 15, 2024
44 of 45 checks passed
@timhoffm
Copy link
Member

Thanks @Impaler343 and congratulations on your first contribution to Matplotlib! 🎉 We hope to see you back.

@Impaler343 Impaler343 deleted the doc-change branch March 15, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants