Skip to content

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

Merged
mhvk merged 2 commits into
astropy:mainfrom
neutrinoceros:units/compat/numpy_2_3_matvec_vecmat
Nov 27, 2024
Merged

BUG: add missing ufunc helpers for np.matvec and np.vecmat#17464
mhvk merged 2 commits into
astropy:mainfrom
neutrinoceros:units/compat/numpy_2_3_matvec_vecmat

Conversation

@neutrinoceros

@neutrinoceros neutrinoceros commented Nov 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 labels Nov 27, 2024
@github-actions

github-actions Bot commented Nov 27, 2024

Copy link
Copy Markdown
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

@mhvk mhvk left a comment •
edited
Loading

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.

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

neutrinoceros commented Nov 27, 2024

Copy link
Copy Markdown
Contributor Author

test added

mhvk
mhvk reviewed Nov 27, 2024
View reviewed changes

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

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!

Comment thread astropy/units/tests/test_quantity_non_ufuncs.py Outdated
assert_array_equal(n2, expected2)


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

@mhvk mhvk Nov 27, 2024

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.

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

@neutrinoceros neutrinoceros Nov 27, 2024

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.

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

neutrinoceros commented Nov 27, 2024

Copy link
Copy Markdown
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

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

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

Projects

None yet

Milestone

v7.0.1

Development

Successfully merging this pull request may close these issues.

2 participants

@neutrinoceros @mhvk