8000 BUG: add missing ufunc helpers for `np.matvec` and `np.vecmat` by neutrinoceros · Pull Request #17464 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

BUG: add missing ufunc helpers for np.matvec and np.vecmat #17464

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 27, 2024
Merged

BUG: add missing ufunc helpers for np.matvec and np.vecmat #17464

merged 2 commits into from
Nov 27, 2024

Conversation

neutrinoceros
Copy link
Contributor
@neutrinoceros neutrinoceros commented Nov 27, 2024 •
edited
Loading

Description

example logs
xref: numpy/numpy#25675

given how small the change, I think we should backport to 7.0.1

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@neutrinoceros neutrinoceros added this to the v7.0.1 milestone Nov 27, 2024
@neutrinoceros neutrinoceros added Affects-dev PRs and issues that do not impact an existing Astropy release numpy-dev 💤 backport-v7.0.x on-merge: backport to v7.0.x labels Nov 27, 2024
@github-actions GitHub Actions
Copy link
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@neutrinoceros neutrinoceros force-pushed the units/compat/numpy_2_3_matvec_vecmat branch from 5ae825c to 57e713a Compare November 27, 2024 08:19
@neutrinoceros neutrinoceros changed the title BUG: add missing ufunc helpers for np.matvec and np.vecmat BUG: add missing ufunc helpers for np.matvec and np.vecmat Nov 27, 2024
mhvk
mhvk reviewed Nov 27, 2024
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! I could have thought about this myself...

One request: would you be able to add tests? In astropy/units/tests/test_quantity_ufuncs.py (after matmul/vecdot) and in astropy/utils/masked/test_masked.py, adding to or with the existing vecmat and matvec tests that use @.

p.s. And, yes, should be backported - we always try to ensure the current version will run with latest numpy.

@mhvk mhvk added no-changelog-entry-needed and removed Affects-dev PRs and issues that do not impact an existing Astropy release labels Nov 27, 2024
@neutrinoceros neutrinoceros force-pushed the units/compat/numpy_2_3_matvec_vecmat branch from 57e713a to 1303114 Compare November 27, 2024 15:59
@neutrinoceros
Copy link
Contributor Author

test added

mhvk
mhvk reviewed Nov 27, 2024
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

See note about location. Also, please do add a test in utils/masked (see previous review message) - it should not require explicitly adding to anything, but we need to test that!

 < 8000 /div>

astropy/units/tests/test_quantity_non_ufuncs.py Outdated
@@ -2702,6 +2703,29 @@ def test_vector_norm(self):
assert_array_equal(n2, expected2)


@pytest.mark.skipif(NUMPY_LT_2_3, reason="np.matvec and np.vecmat are new in NumPy 2.3")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be annoying, but this should be in test_quantity_ufunc (gufuncs are there too...), with matmul and vecdot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no annoyance at all. Thanks for your attention to details !

@neutrinoceros neutrinoceros force-pushed the units/compat/numpy_2_3_matvec_vecmat branch from 1303114 to 727094b Compare November 27, 2024 17:12
@neutrinoceros
Copy link
Contributor Author

it should not require explicitly adding to anything, but we need to test that!

I'm actually getting unexpected errors from my test, and I'm running out of time to fix them today, so I just pushed a broken test for now.

@mhvk mhvk force-pushed the units/compat/numpy_2_3_matvec_vecmat branch from 727094b to cb64713 Compare November 27, 2024 19:03
mhvk
mhvk approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Since there didn't seem to be much wrong with the test you wrote, I force-pushed a quick fix. With this, it all looks good, so approving and enabling merge.

@mhvk mhvk enabled auto-merge November 27, 2024 19:04
@mhvk mhvk merged commit 410dd59 into astropy:main Nov 27, 2024
25 of 28 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Nov 27, 2024
@mhvk @meeseeksmachine
Backport PR astropy#17464: BUG: add missing ufunc helpers for `np.mat…
310e54e 
…vec` and `np.vecmat`
@meeseeksmachine meeseeksmachine mentioned this pull request Nov 27, 2024
Merged
@neutrinoceros neutrinoceros deleted the units/compat/numpy_2_3_matvec_vecmat branch November 27, 2024 19:29
@mhvk mhvk mentioned this pull request Nov 27, 2024
Merged
neutrinoceros added a commit that referenced this pull request Nov 28, 2024
@neutrinoceros
Merge pull request #17466 from meeseeksmachine/auto-backport-of-pr-17…
193447b 
…464-on-v7.0.x

Backport PR #17464 on branch v7.0.x (BUG: add missing ufunc helpers for `np.matvec` and `np.vecmat`)
This was referenced Dec 9, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk approved these changes

Assignees
No one assigned
Labels
no-changelog-entry-needed numpy-dev units utils 💤 backport-v7.0.x on-merge: backport to v7.0.x
Projects
None yet
Milestone
v7.0.1
Development

Successfully merging this pull request may close these issues.
2 participants
@neutrinoceros @mhvk
0