FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixing various spelling errors by john-science · Pull Request #28569 · numpy/numpy · GitHub

/ numpy Public

Fixing various spelling errors - #28569

Merged
ngoldbaum merged 8 commits into
numpy:mainfrom
john-science:fix_spelling
Mar 25, 2025
Merged

ngoldbaum merged 8 commits into
numpy:mainfrom
john-science:fix_spelling

Conversation

Copy link
Copy Markdown
Contributor

This is just a simple cleanup PR. I found various spelling errors and fixed them.

And then I went hunting for spelling errors and found a few more.

I have not changed any variable names here, this is all just docs and docstrings. So this should be a very safe change.

tylerjereddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some of these look "ok," but others look like automatic replacements that don't belong--I've pointed out a few from the latter category from a quick scan.

Comment thread doc/changelog/1.21.0-changelog.rst Outdated
Comment thread doc/changelog/1.21.0-changelog.rst Outdated
Comment thread doc/changelog/1.21.0-changelog.rst Outdated
Comment thread doc/changelog/1.22.0-changelog.rst Outdated
Comment thread doc/source/release/1.22.0-notes.rst Outdated

charris commented Mar 23, 2025

Copy link
Copy Markdown
Member

The f2c stuff is generated and should be left alone.

Comment thread doc/changelog/1.21.0-changelog.rst Outdated
Comment thread doc/changelog/1.21.0-changelog.rst Outdated
Comment thread doc/changelog/1.21.0-changelog.rst Outdated
Comment thread doc/changelog/1.22.0-changelog.rst Outdated
Comment thread doc/source/release/1.22.0-notes.rst Outdated

Copy link
Copy Markdown
Contributor Author

The f2c stuff is generated and should be left alone.

Okay, reverted.

Copy link
Copy Markdown
Contributor Author

This error seems erroneous to me:

  E       AssertionError: 
  E       Arrays are not equal
  E       
  E       Mismatched elements: 2 / 10 (20%)
  E       Max absolute difference among violations: 1208103775
  E       Max relative difference among violations: 32749.
  E        ACTUAL: array([[1208103775,      32750,          2,          3,          4],
  E              [         5,          6,          7,          8,          9]],
  E             dtype=int32)
  E        DESIRED: array([[0, 1, 2, 3, 4],
  E              [5, 6, 7, 8, 9]])
  
  POINTER    = <built-in function POINTER>
  a          = array([[ -217879408,       32745, -1231388384,       32745,           4],
         [          5,           6,           7,          48,           0]],
        dtype=int32)

  numpy/tests/test_ctypeslib.py:218: AssertionError

That does not seem related to this PR. I expect re-running the tests would make them pass again. (Sorry if I am wrong. This is just my guess.)

If that IS the case, perhaps we should open a bug ticket for intermittently failing CI.

charris commented Mar 24, 2025

Copy link
Copy Markdown
Member

You can ignore the PyPy failure, it is a known bug in PyPy.

ngoldbaum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Changing the error messages is technically a backward compatibility issue - can you add a release note for the off chance someone is using the error message in a test like the numpy tests were?

ngoldbaum added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Changing the error messages is technically a backward compatibility issue - can you add a release note for the off chance someone is using the error message in a test like the numpy tests were?

I have added a release note. I tried to word it in keeping with other release notes. But feel free to holler if you'd like it worded differently.

Copy link
Copy Markdown
Member

There was a spelling error in the release note 🙃 so I fixed that and made it a little less wordy. Will merge shortly.

Copy link
Copy Markdown
Member

Actually I think @tylerjereddy needs to give it another pass because of branch protection

Comment thread doc/source/release/1.22.0-notes.rst Outdated
============

Use delimiter rather than delimitor as kwarg in mrecords
Use delimiter rather than delimiter as kwarg in mrecords

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this still needs to be reverted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reverted

tylerjereddy dismissed their stale review March 25, 2025 17:55

most comments addressed

Comment thread doc/source/release/1.22.0-notes.rst Outdated

Copy link
Copy Markdown
Contributor

Actually I think @tylerjereddy needs to give it another pass because of branch protection

I think I found one more case that should be reverted, but I've dismissed the blocking review apart from that.

Copy link
Copy Markdown
Member

Thanks @john-science!

ngoldbaum merged commit 2a6f4f0 into numpy:main Mar 25, 2025
john-science deleted the fix_spelling branch March 25, 2025 22:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

03 - Maintenance 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL