PERF: Use a C Union in maybe_convert_numeric
State: closed

- [ ] closes #xxxx (Replace xxxx with the GitHub issue number)
- [ ] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [ ] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [ ] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [ ] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.

```
       before           after         ratio
     [22de62ca]       [3a53d05b]
                      <maybe-convert-less-mem>
-            743M             673M     0.91  inference.MaybeConvertNumeric.peakmem_convert('float64')
-            986M             836M     0.85  inference.MaybeConvertNumeric.peakmem_convert('int64')
-            986M             836M     0.85  inference.MaybeConvertNumeric.peakmem_convert('uint64')
-        685±10ms         500±30ms     0.73  inference.MaybeConvertNumeric.time_convert('bool')
-       2.20±0.1s       1.51±0.07s     0.69  inference.MaybeConvertNumeric.time_convert('uint64')
-            428M             278M     0.65  inference.MaybeConvertNumeric.peakmem_convert('bool')
-      2.07±0.08s       1.23±0.06s     0.60  inference.MaybeConvertNumeric.time_convert('int64')
```

Labels:
name: Performance
description: Memory or execution speed performance
name: Mothballed
description: Temporarily-closed PR the author plans to return to

Comments:
ballpark how much improvement?
> ballpark how much improvement?

Updated the OP.

Note, though, that I've turned off the fastpath for ints in my benchmarks.

I would like @WillAyd 's thoughts on this, though to make sure I'm not doing something sketchy with the union or the casting in the Cython code, though.
Reworked the approach to upcast when necessary. 
There's still a lot of boilerplate, though.
(I'm hoping that we can refactor some of it to live on the ``Seen`` object so that it can be shared with ``maybe_convert_objects``)

Perf is slightly better than the old version.
(I suspect that the union array approach could be faster if we did a zero copy creation of the ndarray from the union array, but that probably makes it way too complex). 

```
       before           after         ratio
     [0cedcbf6]       [fd205e6a]
                      <maybe-convert-less-mem>
-        664±20ms          525±7ms     0.79  inference.MaybeConvertNumeric.time_convert('bool')
-            743M             583M     0.78  inference.MaybeConvertNumeric.peakmem_convert('float64')
-            986M             746M     0.76  inference.MaybeConvertNumeric.peakmem_convert('uint64')
-            986M             746M     0.76  inference.MaybeConvertNumeric.peakmem_convert('int64')
-      2.12±0.04s       1.36±0.01s     0.64  inference.MaybeConvertNumeric.time_convert('uint64')
-      1.95±0.04s       1.24±0.02s     0.64  inference.MaybeConvertNumeric.time_convert('int64')
-            428M             268M     0.63  inference.MaybeConvertNumeric.peakmem_convert('bool')

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.
```
did you ever try out the two-pass approach?  i.e. do inference first, then allocate just one array?

> did you ever try out the two-pass approach? i.e. do inference first, then allocate just one array?

I forgot that ``maybe_convert_numeric`` also can process strings with the ``floatify`` function, so we would need to store the parsed values somewhere, or do a double parse (can be expensive if all strings, which I don't think is uncommon).

Right now, we use just 1 array, but astype it whenever we need to upcast. IMO, this is about the same complexity as the other approach, a lot of the diff just comes from me moving logic to the ``Seen`` class, so ``maybe_convert_objects`` will be easier to port to this impl.
I'll take another pass at it this weekend. @jbrockmendel care to take a look as well?
@WillAyd Did you have a chance to look at this? No rush, but I'd like to get this in soon.
Will take another look today.
OK, looking at this now.  Are the asv results from feb 6 still accurate-ish?
> OK, looking at this now. Are the asv results from feb 6 still accurate-ish?

Benchmarks still show roughly the same improvement.
@jbrockmendel 
Any chance you can have a look at this soon?

I'm trying to get this in before your deprecation PRs for inferring stuff to pyarrow.
How important is this performance boost in the grand scheme of things? I still have some pretty big concerns about the design pattern, loss of type safety and future maintenance burden of this, so I'm not sold on this being merge-able as is
I'll take another look, definitely ping me again (ATM on slack is most likely to not fall through the cracks) if i dont get to it by the weekend.

My knee-jerk reaction mirrors Wills last comment.
> How important is this performance boost in the grand scheme of things? I still have some pretty big concerns about the design pattern, loss of type safety and future maintenance burden of this, so I'm not sold on this being merge-able as is

The main thing it should be boosting is the speed of ``to_numeric``. If this was ported to ``maybe_convert_objects`` as well, I think it would also boost the perf of constructors and ``infer_objects`` I guess.

The boost isn't really big, but I think the (on average) 20% speed increase and 20% memory usage decrease is not bad.

Happy to let this sit for a while, though, if you're close to making a proposal on changing _libs to Rust(which would guarantee the safetyness of this I guess).
Looking at this again I'm still really uncomfortable with it.  I think a two-pass approach where the first pass does the inference and the second pass does the casting would be way simpler and get all the memory benefits.  How much would that cost perf-wise?
Seems like there's lukewarm reception to this so far and has stalled. I'll mothball for now but feel free to reopen if interested in a different approach