ENH: Add ufunc for np.char.isalpha #24835

lysnikolaou · 2023-09-29T11:22:29Z

No description provided.

lysnikolaou · 2023-09-29T11:27:29Z

One problem with the current state of this PR that I can't really find a solution to is this:

The isalpha ufunc itself works correctly, both when called directly, and when called from the np.char module with an ndarray.

$ export PYTHONPATH="/Users/lysnikolaou/repos/python/numpy/build-install/usr/lib/python3.11/site-packages"
🐍 Launching Python with PYTHONPATH="/Users/lysnikolaou/repos/python/numpy/build-install/usr/lib/python3.11/site-packages"
$ /usr/bin/env python -P
Python 3.11.3 (tags/v3.11.3:f3909b8bc8, May 29 2023, 12:51:44) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> a = np.array([['abc', 'def'], ['world', ' he']])
>>> np.core.multi
np.core.multiarray np.core.multiply( 
>>> np.core.multiarray.isalpha(a)
array([[ True,  True],
       [ True, False]])
>>> np.char.isalpha(a)
array([[ True,  True],
       [ True, False]])

However, I'm getting a value error when passing a chararray to it.

$ export PYTHONPATH="/Users/lysnikolaou/repos/python/numpy/build-install/usr/lib/python3.11/site-packages"
🐍 Launching Python with PYTHONPATH="/Users/lysnikolaou/repos/python/numpy/build-install/usr/lib/python3.11/site-packages"
$ /usr/bin/env python -P
Python 3.11.3 (tags/v3.11.3:f3909b8bc8, May 29 2023, 12:51:44) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> a = np.char.array([['abc', 'def'], ['world', ' he']])
>>> np.core.multiarray.isalpha(a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/lysnikolaou/repos/python/numpy/build-install/usr/lib/python3.11/site-packages/numpy/core/defchararray.py", line 2099, in __array_finalize__
    raise ValueError("Can only create a chararray from string data.")
ValueError: Can only create a chararray from string data.

It seems that the bool array returned from the ufunc is somehow getting cast to a chararray, which is the input type. Is that something that should be happening? Am I missing something more obvious?

seberg · 2023-09-29T11:39:21Z

Is that something that should be happening?

Yes, that should be happening. Unlike a custom function, UFuncs will correctly preserve subclass information.

Could probably add something like (untested):

def __array_prepare__(self, arr, context=None):
    if arr.dtype in "US":
        return arr.view(type(self))
    return arr

to make it work, I doubt that has any negative side-effects. Maybe __array_wrap__ but I suspect __array_prepare__ might be more targeeted even. (Alternatively, there is a point why we don't just return a normal array when this happens.)

lysnikolaou · 2023-09-29T12:05:29Z

Is there a way to tell the ufunc to force a specific return type? Because __array_finalize__ allows all of SUbc, I fear that it might break something if we change it in the chararray itself.

seberg · 2023-09-29T12:31:19Z

You can use subok=False only (or pass out=), but what is the problem with adding an array prepare like that? I never suggested to losen __array_finalize__.

EDIT: Or does __array_prepare__ kick in only after this error is given?

lysnikolaou · 2023-09-29T13:07:58Z

The same is still happening after applying this:

numpy on  string-ufuncs-isalpha [$!?] is 📦 v2.0.0.dev0 via 🐍 pyenv mambaforge (venv) 
❯ git diff  
diff --git a/numpy/core/defchararray.py b/numpy/core/defchararray.py
index c3968bc43..2c2c7c955 100644
--- a/numpy/core/defchararray.py
+++ b/numpy/core/defchararray.py
@@ -2093,6 +2093,11 @@ def __new__(subtype, shape, itemsize=1, unicode=False, buffer=None,
         _globalvar = 0
         return self
 
+    def __array_prepare__(self, arr, context=None):
+        if arr.dtype.char in "US":
+            return arr.view(type(self))
+        return arr
+
     def __array_finalize__(self, obj):
         # The b is a special case because it is used for reconstructing.
         if not _globalvar and self.dtype.char not in 'SUbc':

lysnikolaou · 2023-10-02T11:11:21Z

@mattip @seberg This appears to be okay now. I added an __array_wrap__ which just returns the arr that's being passed in. I don't have a deep enough understanding of the ufunc mechanism yet to figure out if this is okay like this, so I'd like a review please.

mattip · 2023-10-02T12:55:55Z

Nice. I think the __array_wrap__ is correct. Could you rerun the benchmarks?

lysnikolaou · 2023-10-02T13:26:21Z

Could you rerun the benchmarks?

Similar results:

| Change   | Before [eabefa4e] <main>   | After [54ef4672] <string-ufuncs-isalpha>   |   Ratio | Benchmark (Parameter)                                   |
|----------|----------------------------|--------------------------------------------|---------|---------------------------------------------------------|
| -        | 7.64±0.04μs                | 2.30±0.1μs                                 |     0.3 | bench_core.NumPyChar.time_isalpha_small_list_big_string |
| -        | 1.34±0.02ms                | 5.15±0.2μs                                 |     0   | bench_core.NumPyChar.time_isalpha_big_list_small_string |

numpy/core/defchararray.py

lysnikolaou · 2023-10-03T13:02:28Z

@ngoldbaum Is stringdtype currently broken on main? I'm trying to test this and building stringdtype fails both with this branch and with the numpy nightly build. Maybe I'm missing something.

ngoldbaum · 2023-10-03T14:33:40Z

I'm trying to test this and building stringdtype fails both with this branch and with the numpy nightly build

I don't see this when I build stringdtype using numpy's main branch or this PR. Can you open an issue over on the numpy-user-dtypes repo with more detail about what's happening on your setup? Maybe it's just a documentation thing but if it isn't I'd like to know what the problem is.

FWIW, this does break isalpha for stringdtype because the dtype doesn't have an implementation of the ufunc yet, but I think that's expected. No need for numpy to handle that I think, I can fix it in stringdtype once this is merged.

lysnikolaou · 2023-10-03T16:17:22Z

Sounds good. I'll try to recreate and will open a PR.

@mattip @seberg I've got a follow-up PR ready that adds support for np.add. Can we merge this so that I can open that PR?

seberg · 2023-10-04T08:18:46Z

numpy/core/src/umath/string_ufuncs.cpp

+     * however it may be that this should be moved into `auxdata` eventually,
+     * which may also be slightly faster/cleaner (but more involved).
+     */
+    int len = steps[0] / sizeof(character);


I had ignored it, assuming this was a first iteration for testing. Yes, I know it adds some boilerplate, but we should probably just create the ufunc without any loops and then add them the same way as the string equality is done.

Then change the signature of this loop and the above comment actually can make sense.

Right now, you should get crashes for some sliced arrays arr = arr[::10]

@seberg I think I've got something better now. Can you check again and let me know whether I've missed anything else?

seberg

Generally LGTM now, I would like the rstrip template parameter to be removed from isalpha mainly.
I will let Matti take a final quick look when you feel my comments are addressed well enough.

seberg · 2023-10-04T12:38:08Z

numpy/core/code_generators/generate_umath.py

@@ -1137,6 +1144,13 @@ def english_upper(s):
          TD(O),
          signature='(n?,k),(k,m?)->(n?,m?)',
          ),
+'isalpha' :


Just to make sure: this only shows up in the internal ufunc namespace? (actually I guess tests would fail if not)

That's correct, yes.

seberg · 2023-10-04T12:39:11Z

numpy/core/code_generators/generate_umath.py

+          docstrings.get('numpy.core.umath.isalpha'),
+          None,
+          [TypeDescription(U, EmptyFunctionTypeDescr, U, '?'),
+           TypeDescription(S, EmptyFunctionTypeDescr, S, '?')],


Could just leave this empty, instead of the EmptyFunctionTypeDescr? If not, fine, but maybe add a comment to EmptyFunctionTypeDescr to explain what it is used for.

Yes, this ensures it shows up in .types and it may actually be a reasonable way to ensure that in practice. But TBH, it is half-private anyway, so maybe we can just ignore it for now.

This wasn't possible before cause of a syntax error on empty arrays under MSVC, but I wrote some code so that it's okay.

numpy/core/src/umath/string_ufuncs.cpp

numpy/core/defchararray.py

ngoldbaum · 2023-10-04T14:13:04Z

I'm glad it was relatively straightforward to add support for string ufuncs besides the logic operators, looking forward to improved string operator performance! 🚀

mattip

A few comments, and some documentation nits.

numpy/core/code_generators/generate_umath.py

mattip · 2023-10-04T14:28:55Z

numpy/core/code_generators/generate_umath.py

+                    return -1;
+                }}
+                f = PyUFunc_FromFuncAndDataAndSignatureAndIdentity(
+                    NULL, NULL, NULL, {nloops},


Only this line is different between the two stanzas. It should be possible to refactor this

to select on an attribute of uf rather than have a separate empty list, passing it around seems awkward. Something like what I suggested above?

extra points for tweaking fmt for an empty uf rather than rewriting the whole thing

numpy/core/code_generators/ufunc_docstrings.py

lysnikolaou · 2023-10-04T15:13:29Z

@mattip @seberg Thanks both for the reviews. I've addressed all comments.

mattip · 2023-10-04T16:31:36Z

Thanks @lysnikolaou

lysnikolaou · 2023-12-11T12:22:59Z

@charris This was the first one of the PRs adding ufuncs for np.char and we'd like to backport this to 1.x. Is it okay to tag you in all of the other PRs that need backporting as well? I'm not sure about the exact process here, so I'd need a few pointers.

seberg · 2023-12-11T12:51:27Z

While it seems nice that these are backportable. I don't really think they should be backported for a 1.26.x bug-fix release. These are quite large changes, and chances are some behavior is changed.

charris · 2023-12-11T15:03:53Z

Agree with @seberg here, we don't usually add new features in patch releases.

lysnikolaou · 2023-12-11T16:37:38Z

Got it, thanks both for your input!

Add ufunc for np.char.isalpha

ef5e545

charris changed the title ~~Add ufunc for np.char.isalpha~~ ENH: Add ufunc for np.char.isalpha Sep 29, 2023

Fix ufuncs output type cast for chararrays

34a5a20

Add isalpha benchmarks

54ef467

seberg reviewed Oct 2, 2023

View reviewed changes

numpy/core/defchararray.py Show resolved Hide resolved

Use same function is __array_prepare__ and __array_wrap__

222d445

seberg reviewed Oct 4, 2023

View reviewed changes

lysnikolaou mentioned this pull request Oct 4, 2023

stringdtype fails to build on Linux numpy/numpy-user-dtypes#90

Closed

lysnikolaou added 3 commits October 4, 2023 13:29

Register isalpha like other ufuncs and add loop in string_ufuncs.cpp

7c20659

Fix docstring

363d160

Fix syntax error on Windows by populating ufunc functions with NULL

e767771

lysnikolaou force-pushed the string-ufuncs-isalpha branch from 3c6d570 to e767771 Compare October 4, 2023 12:19

seberg reviewed Oct 4, 2023

View reviewed changes

lysnikolaou added 5 commits October 4, 2023 15:08

Do not create functions arrays for ufuncs without loops

4d07d7b

Address feedback - add comments and remove rstrip

8200573

Minor refactor of ordering/naming of arguments in string ufuncs loops

b51eb71

Clean-up of generate_umath.py

d703459

Fix lint errors

a3bfa73

Fix tests // error in refactoring

6405f9f

mattip reviewed Oct 4, 2023

View reviewed changes

Address feedback - improve docs and code in generate_umath

7f83385

mattip merged commit 64fc516 into numpy:main Oct 4, 2023
48 of 50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add ufunc for np.char.isalpha #24835

ENH: Add ufunc for np.char.isalpha #24835

lysnikolaou commented Sep 29, 2023

lysnikolaou commented Sep 29, 2023 •

edited

Loading

seberg commented Sep 29, 2023

lysnikolaou commented Sep 29, 2023

seberg commented Sep 29, 2023 •

edited

Loading

lysnikolaou commented Sep 29, 2023

lysnikolaou commented Oct 2, 2023

mattip commented Oct 2, 2023

lysnikolaou commented Oct 2, 2023

lysnikolaou commented Oct 3, 2023 •

edited

Loading

ngoldbaum commented Oct 3, 2023

lysnikolaou commented Oct 3, 2023

seberg Oct 4, 2023

lysnikolaou Oct 4, 2023

seberg left a comment

seberg Oct 4, 2023

lysnikolaou Oct 4, 2023

seberg Oct 4, 2023

lysnikolaou Oct 4, 2023

ngoldbaum commented Oct 4, 2023

mattip left a comment

mattip Oct 4, 2023

lysnikolaou commented Oct 4, 2023

mattip commented Oct 4, 2023

lysnikolaou commented Dec 11, 2023

seberg commented Dec 11, 2023

charris commented Dec 11, 2023

lysnikolaou commented Dec 11, 2023

ENH: Add ufunc for np.char.isalpha #24835

ENH: Add ufunc for np.char.isalpha #24835

Conversation

lysnikolaou commented Sep 29, 2023

lysnikolaou commented Sep 29, 2023 • edited Loading

seberg commented Sep 29, 2023

lysnikolaou commented Sep 29, 2023

seberg commented Sep 29, 2023 • edited Loading

lysnikolaou commented Sep 29, 2023

lysnikolaou commented Oct 2, 2023

mattip commented Oct 2, 2023

lysnikolaou commented Oct 2, 2023

lysnikolaou commented Oct 3, 2023 • edited Loading

ngoldbaum commented Oct 3, 2023

lysnikolaou commented Oct 3, 2023

seberg Oct 4, 2023

Choose a reason for hiding this comment

lysnikolaou Oct 4, 2023

Choose a reason for hiding this comment

seberg left a comment

Choose a reason for hiding this comment

seberg Oct 4, 2023

Choose a reason for hiding this comment

lysnikolaou Oct 4, 2023

Choose a reason for hiding this comment

seberg Oct 4, 2023

Choose a reason for hiding this comment

lysnikolaou Oct 4, 2023

Choose a reason for hiding this comment

ngoldbaum commented Oct 4, 2023

mattip left a comment

Choose a reason for hiding this comment

mattip Oct 4, 2023

Choose a reason for hiding this comment

lysnikolaou commented Oct 4, 2023

mattip commented Oct 4, 2023

lysnikolaou commented Dec 11, 2023

seberg commented Dec 11, 2023

charris commented Dec 11, 2023

lysnikolaou commented Dec 11, 2023

lysnikolaou commented Sep 29, 2023 •

edited

Loading

seberg commented Sep 29, 2023 •

edited

Loading

lysnikolaou commented Oct 3, 2023 •

edited

Loading