Skip to content

ENH: add fast path for str(scalar_int) #23746

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
charris merged 2 commits into from
May 13, 2023
Merged

ENH: add fast path for str(scalar_int) #23746

charris merged 2 commits into from
May 13, 2023

Conversation

@mattip
Copy link
Member

@mattip mattip commented May 10, 2023

Create a fast path for str(scalar_int). The call to str(x) when x is an int scalar goes to genint_type_str, which calls PyObject_Str(gentype_generic_method(self, NULL, NULL, "item")). The gentype_generic_method creates a 0d array of the scalar (allocating a PyArrayObject with 1 data item), copies the item into the new 0d array, then calls arr.item() to get the int value, then wraps that in a PyLongObject.

It is faster to avoid all that and use scalar_value to get the void * value, then cast that to the proper int type before wrapping it in a PyLongObject.

I also added a benchmark to prove there is a speedup.

       before           after         ratio
     [181c15b2]       [fcf8a88e]
     <main>           <scalar-str>
-      29.3±0.3μs      27.8±0.09μs     0.95  bench_scalar.ScalarStr.time_addition('complex128')
-      15.9±0.3μs       15.0±0.2μs     0.94  bench_scalar.ScalarStr.time_addition('float64')
-      42.6±0.9μs       11.7±0.1μs     0.27  bench_scalar.ScalarStr.time_addition('int32')
-      42.1±0.9μs      11.2±0.06μs     0.27  bench_scalar.ScalarStr.time_addition('int16')
-      41.7±0.8μs       11.0±0.2μs     0.26  bench_scalar.ScalarStr.time_addition('int64')

This was motivated by a user who noticed that [str(int(x)) for x in numpy.arange(10000)] is much faster than [str(x) for x in numpy.arange(10000)]

mattip
mattip commented May 10, 2023
View reviewed changes
numpy/core/src/multiarray/scalartypes.c.src
break;
default:
item = gentype_generic_method(self, NULL, NULL, "item");
break;
Copy link
Member Author

@mattip mattip May 10, 2023

Choose a reason for hiding this comment

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

A fallback for some as-yet-unknown user defined int dtype

Copy link
Member

@seberg seberg May 11, 2023

Choose a reason for hiding this comment

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

We need to generate a function for each type anyway. I don't think that str(self.item()) is a good default to begin with.

That said, I don't mind just doing this, I will just delete it again on main in the next 2 months hopefully. (The work should already be in the repr PR, and its time to push that after branching.)

mattip
mattip commented May 10, 2023
View reviewed changes
numpy/core/tests/test_arrayprint.py


def test_structure_format(self):
def test_structure_format_mixed(self):
Copy link
Member Author

@mattip mattip May 10, 2023

Choose a reason for hiding this comment

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

I split this test into 3, since the other test functions were not related to this one. I hit this by accident when I was trying out a different version of the code and printing was failing.

seberg
seberg approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Not sure I wouldn't prefer a type specific path (just because its the pattern used I think). But, this will generate a merge conflict with larger changes anyway, so I don't care.

So, lets put it in for 1.25.x.

numpy/core/src/multiarray/scalartypes.c.src
break;
default:
item = gentype_generic_method(self, NULL, NULL, "item");
break;
Copy link
Member

@seberg seberg May 11, 2023

Choose a reason for hiding this comment

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

We need to generate a function for each type anyway. I don't think that str(self.item()) is a good default to begin with.

That said, I don't mind just doing this, I will just delete it again on main in the next 2 months hopefully. (The work should already be in the repr PR, and its time to push that after branching.)

@seberg seberg added this to the 1.25.0 release milestone May 11, 2023
@mattip
Copy link
Member Author

mattip commented May 11, 2023

Ahh, type-specific __str__ methods for scalars are already part of the dtype refactor? Sorry, I didn't know that...

@ngoldbaum
Copy link
Member

ngoldbaum commented May 11, 2023

Ahh, type-specific str methods for scalars are already part of the dtype refactor?

You could define a dtype with a scalar that isn’t convertible to a string, but basic things like array printing wouldn’t work. We could probably add a check to the dtype registration that makes sure the scalar has a __str__ and __repr__ and raises an error otherwise.

@charris charris merged commit 81caed6 into numpy:main May 13, 2023
@charris
Copy link
Member

charris commented May 13, 2023

Thanks Matti.

@seberg seberg mentioned this pull request Jul 23, 2023
Closed
thesamesam added a commit to thesamesam/numpy that referenced this pull request Jul 23, 2023
@thesamesam
BUG: Fix C types in scalartypes
97c1500 
numpy#23746 introduced a fast path for scalar
int conversions, but the map between Python types and C types was subtly
wrong.

This fixes tests on at least ppc32 (big-endian).

Many thanks to Sebastian Berg for debugging this with me and pointing out
what needed to be fixed.

Bug: numpy#24239
Fixes: 81caed6
thesamesam added a commit to thesamesam/numpy that referenced this pull request Jul 23, 2023
@thesamesam
BUG: Fix C types in scalartypes
f5eb12c 
numpy#23746 introduced a fast path for scalar
int conversions, but the map between Python types and C types was subtly
wrong.

This fixes tests on at least ppc32 (big-endian).

Many thanks to Sebastian Berg for debugging this with me and pointing out
what needed to be fixed.

Closes numpy#24239.

Fixes: 81caed6
@thesamesam thesamesam mentioned this pull request Jul 23, 2023
Merged
seberg pushed a commit to thesamesam/numpy that referenced this pull request Jul 25, 2023
@thesamesam @seberg
BUG: Fix C types in scalartypes
d9c0c96 
numpy#23746 introduced a fast path for scalar
int conversions, but the map between Python types and C types was subtly
wrong.

This fixes tests on at least ppc32 (big-endian).

Many thanks to Sebastian Berg for debugging this with me and pointing out
what needed to be fixed.

Closes numpy#24239.

Fixes: 81caed6
charris pushed a commit to charris/numpy that referenced this pull request Jul 30, 2023
@thesamesam @charris
BUG: Fix C types in scalartypes
8a731a5 
numpy#23746 introduced a fast path for scalar
int conversions, but the map between Python types and C types was subtly
wrong.

This fixes tests on at least ppc32 (big-endian).

Many thanks to Sebastian Berg for debugging this with me and pointing out
what needed to be fixed.

Closes numpy#24239.

Fixes: 81caed6
@charris charris mentioned this pull request Jul 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg approved these changes

Assignees

No one assigned

Labels

01 - Enhancement

Projects

None yet

Milestone

1.25.0 release

Development

Successfully merging this pull request may close these issues.

4 participants

@mattip @ngoldbaum @charris @seberg