Skip to content

Add new (2.0) string ufuncs in np.char to UNSUPPORTED_UFUNCS - #15699

Merged
dhomeier merged 2 commits into
astropy:mainfrom
dhomeier:np.char_ufuncs
Dec 9, 2023
Merged

dhomeier merged 2 commits into
astropy:mainfrom
dhomeier:np.char_ufuncs

Conversation

@dhomeier

@dhomeier dhomeier commented Dec 8, 2023 •

Copy link
Copy Markdown
Contributor

Description

Additional ufuncs in np.char not yet covered by units.quantity_helpers; added to UNSUPPORTED_UFUNCS as none of them make sense for quantities.

Fixes part of #15690 – actually the above were not mentioned there or showed up in any test agains numpy-2.0 since TestUfuncHelpers.test_coverage is skipped with scipy installed.

Tagging @neutrinoceros

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

@github-actions

github-actions Bot commented Dec 8, 2023

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?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • 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

Copy link
Copy Markdown
Contributor

Hi @dhomeier, there's already an active PR to resolve this problem (#15691). If you have reasons to reject it can you please let me know there with a review ? Thanks !

@dhomeier
dhomeier requested a review from mhvk December 8, 2023 17:48
@dhomeier

dhomeier commented Dec 8, 2023 •

Copy link
Copy Markdown
Contributor Author

@neutrinoceros you're right, this is a separate failure that is not actually covered by CI, see updated comment above; you should be able to reproduce it in your setup by running pip uninstall scipy and then test_quantity_ufuncs.
But since the fix here is rather trivial, feel free to pull it int your PR.

@neutrinoceros

Copy link
Copy Markdown
Contributor

Oh I see, indeed that's a separate problem, so I see no harm in keeping our PRs separate. Thank you !

@neutrinoceros

Copy link
Copy Markdown
Contributor

Indeed I see a difference when running pytest astropy/units/tests/test_quantity_ufuncs.py::TestUfuncHelpers::test_coverage -vvv against main:

E       AssertionError: assert {<ufunc '_rstrip_whitespace'>, <ufunc '_lstrip_chars'>, <ufunc 'startswith'>, <ufunc '_strip_whitespace'>, <ufunc 'isdecimal'>, <ufunc 'isnumeric'>, <ufunc 'endswith'>, <ufunc '_rstrip_chars'>, <ufunc '_lstrip_whitespace'>, <ufunc '_strip_chars'>, <ufunc 'find'>, <ufunc 'str_len'>, <ufunc 'isdigit'>, <ufunc 'count'>, <ufunc 'rfind'>, <ufunc 'isspace'>} == set()
E         Extra items in the left set:
E         <ufunc '_rstrip_whitespace'>
E         <ufunc '_lstrip_chars'>
E         <ufunc 'isnumeric'>
E         <ufunc 'endswith'>
E         <ufunc '_lstrip_whitespace'>
E         <ufunc '_strip_chars'>
E         <ufunc 'isspace'>
E         <ufunc 'startswith'>
E         <ufunc '_strip_whitespace'>
E         <ufunc 'isdecimal'>
E         <ufunc '_rstrip_chars'>
E         <ufunc 'find'>
E         <ufunc 'str_len'>
E         <ufunc 'isdigit'>
E         <ufunc 'count'>
E         <ufunc 'rfind'>
E         Full diff:
E         - set(,
E         - )
E         + {
E         +  <ufunc '_lstrip_chars'>,
E         +  <ufunc '_lstrip_whitespace'>,
E         +  <ufunc '_rstrip_chars'>,
E         +  <ufunc '_rstrip_whitespace'>,
E         +  <ufunc '_strip_chars'>,
E         +  <ufunc '_strip_whitespace'>,
E         +  <ufunc 'count'>,
E         +  <ufunc 'endswith'>,
E         +  <ufunc 'find'>,
E         +  <ufunc 'isdecimal'>,
E         +  <ufunc 'isdigit'>,
E         +  <ufunc 'isnumeric'>,
E         +  <ufunc 'isspace'>,
E         +  <ufunc 'rfind'>,
E         +  <ufunc 'startswith'>,
E         +  <ufunc 'str_len'>,
E         + }

and with this branch

E       AssertionError: assert {<ufunc '_lstrip_chars'>, <ufunc '_rstrip_whitespace'>, <ufunc '_strip_whitespace'>, <ufunc '_strip_chars'>, <ufunc '_lstrip_whitespace'>, <ufunc '_rstrip_chars'>, <ufunc 'str_len'>, <ufunc 'count'>} == set()
E         Extra items in the left set:
E         <ufunc '_lstrip_chars'>
E         <ufunc '_rstrip_whitespace'>
E         <ufunc '_strip_whitespace'>
E         <ufunc '_strip_chars'>
E         <ufunc '_lstrip_whitespace'>
E         <ufunc '_rstrip_chars'>
E         <ufunc 'str_len'>
E         <ufunc 'count'>
E         Full diff:
E         - set(,
E         - )
E         + {
E         +  <ufunc '_lstrip_chars'>,
E         +  <ufunc '_lstrip_whitespace'>,
E         +  <ufunc '_rstrip_chars'>,
E         +  <ufunc '_rstrip_whitespace'>,
E         +  <ufunc '_strip_chars'>,
E         +  <ufunc '_strip_whitespace'>,
E         +  <ufunc 'count'>,
E         +  <ufunc 'str_len'>,
E         + }

but the test fails in all cases ! Do you know what's going on here ? (I rebuilt numpy dev from source right before I wrote this reply)

@dhomeier

dhomeier commented Dec 8, 2023 •

Copy link
Copy Markdown
Contributor Author

Yes, just reproduced that after I force-rebuilt numpy from git main to test your branch
(https://pypi.anaconda.org/scientific-python-nightly-wheels/simple gave me something like 2.0.0.dev0+git20231111.9340fca, maybe "nightly" refers to the lunar night...).
So those must also have been added since; count and str_len are also in np.char, but the _strip* ones (numpy/numpy#25152) sit in umath only – wondering if they should be explicitly added to UNSUPPORTED_UFUNCS as they are obviously private.

@pllim

pllim commented Dec 8, 2023

Copy link
Copy Markdown
Member

Numpy "nightly" isn't really published every Earth night.

@dhomeier

dhomeier commented Dec 8, 2023

Copy link
Copy Markdown
Contributor Author

but the _strip* ones (numpy/numpy#25152) sit in umath only – wondering if they should be explicitly added to UNSUPPORTED_UFUNCS as they are obviously private.

Well, they are the only private ones except one so far:

>>> [u for k, u in np._core.umath.__dict__.items() if isinstance(u, np.ufunc) and k.startswith('_')]
[<ufunc '_ones_like'>, <ufunc '_strip_whitespace'>, <ufunc '_lstrip_whitespace'>, <ufunc '_rstrip_whitespace'>, <ufunc '_strip_chars'>, <ufunc '_lstrip_chars'>, <ufunc '_rstrip_chars'>]

and _ones_like has a helper function.

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

I think it is fine to add also these private ufuncs to the list, especially since we did that already with _ones_likeh, with the logic that if a wrapping function uses those, it will now work (well, will fail appropriately for those string functions!).

So, I'll approve. I won't immediately merge just in case there is an ordering issue, though I'd guess it is best to get this in and rebase the other PR on it. If so, please just go ahead and merge!

p.s. iIf this continues, we can try to be smart and automatically exclude all ufuncs that only works on dtypes "S" and "U".

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

Please merge away ! devdeps CI won't go green until we get this and #15691 in (and as we saw it may not be enough still), but as I'll need to rebase mine anyway I'm happy to go in second !

@dhomeier

dhomeier commented Dec 9, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews! Thought about waiting over the weekend for the _strip functions to get in, but since they are not tested anyway it does not matter.

@dhomeier
dhomeier merged commit 72c0947 into astropy:main Dec 9, 2023
@dhomeier
dhomeier deleted the np.char_ufuncs branch December 9, 2023 10:56
@pllim pllim added this to the v6.0.1 milestone Dec 9, 2023
@pllim

pllim commented Dec 9, 2023

Copy link
Copy Markdown
Member

@dhomeier , please remember milestone and backport label next time. Thanks!

@pllim

pllim commented Dec 9, 2023

Copy link
Copy Markdown
Member

@meeseeksdev backport to v6.0.x

@dhomeier

dhomeier commented Dec 9, 2023

Copy link
Copy Markdown
Contributor Author

Ow, specifically did not set one initially as I did not recall the backport rules for numpy-2 fixes, but then forgot to go through the checklist, sorry.

pllim added a commit that referenced this pull request Dec 11, 2023
…699-on-v6.0.x

Backport PR #15699 on branch v6.0.x (Add new (2.0) string ufuncs in np.char to UNSUPPORTED_UFUNCS)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants