perf: make to_py_scalar 3x faster when argument is str

[MarcoGorelli]

pretty consistent difference here:

main:

In [2]: %timeit nw.to_py_scalar('foo')
468 ns ± 67.9 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [3]: %timeit nw.to_py_scalar('foo')
338 ns ± 37.6 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

here:

In [1]: import narwhals as nw

In [2]: %timeit nw.to_py_scalar('foo')
154 ns ± 23.6 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [3]: %timeit nw.to_py_scalar('foo')
131 ns ± 6.37 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

What type of PR is this? (check all applicable)

• 💾 Refactor
• ✨ Feature
• 🐛 Bug Fix
• 🔧 Optimization
• 📝 Documentation
• ✅ Test
• 🐳 Other

Related issues

• Related issue #
• Closes #

Checklist

• Code follows style guide (ruff)
• Tests added
• Documented the changes

If you have comments or can explain your changes, please do so below.

[MarcoGorelli]

@EdAbati fancy taking a look?

[FBruzzesi]

Commenting from mobile, just have a tiny question :)

[FBruzzesi]

Thanks for the quick adjustment @MarcoGorelli 🙌🏼🚀

[EdAbati]

Thanks @MarcoGorelli, I like to see/learn about ways to speed up code

so checking number.Number early on would cause numpy scalars to not be converted to Python scalars

should we also add (np.int64(1), 1) in the tests to capture this? (at the moment that is indirectly tested in the doctests)

and the rest looks great!

[EdAbati]

Just so I know what to do/review in the future, which kind of methods should be added here?

[MarcoGorelli]

I think all of them to be honest, it's probably simpler to just use import narwhals as nw in all docstrings and forget about this test 😅 i'll make a separate pr

[FBruzzesi]

Kind of related, I am running a test, and getting a raise if None enters this function. Should we add it to non temporal scalar types?

[MarcoGorelli]

very good point, thanks!

[MarcoGorelli]

really useful comments here, thanks both for excellent reviews 🙏