API: Deprecate compact_ints and use_unsigned in read_csv
State: closed

Title is self-explanatory.

xref #12686 - I don't quite understand why these are marked (if at all) as internal to the C engine only, as the benefits for having these options accepted for the Python engine is quite clear based on the documentation I added as well.

Implementation simply just calls the already-written function in `pandas/parsers.pyx` - as it isn't specific to the `TextReader` class, crossing over to grab this function from Cython (instead of duplicating in pure Python) seems reasonable while maintaining that separation between the C and Python engines.


Labels:
name: API Design
name: IO CSV
description: read_csv, to_csv

Comments:
<b>FYI</b>: the `test_compact_ints` test that I deleted was a duplicate of `test_compact_ints_as_recarray` and also did not correctly reflect on the situation of the test data.

## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/13323?src=pr) is **84.22%**

> Merging [#13323](https://codecov.io/gh/pydata/pandas/pull/13323?src=pr) into [master](https://codecov.io/gh/pydata/pandas/branch/master?src=pr) will decrease coverage by **<.01%**

``` diff
@@             master     #13323   diff @@
==========================================
  Files           138        138          
  Lines         50713      50671    -42   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          42715      42676    -39   
+ Misses         7998       7995     -3   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [99e78da...0d56c09](https://codecov.io/gh/pydata/pandas/compare/99e78da19d9d267fd4e30c0a0268c072856df26e...0d56c0909a1533574f5f1090bd4d48441f1ee297)

This is fine to document these. In reality the parser should not be doing this, and these options just add to an already huge API. Better to have this an an option to `to_numeric()` or something. Or the user can simply pass in `dtypes` as appropriate. 

I get that this makes things more 'automatic'.

So I think that we should deprecate these for now.

@jorisvandenbossche ?

@jreback : No issue with simplifying the API, though it did seem like it was a somewhat decent memory-conscientious option.  If it is deprecated, telling them to refer to `dtype` might make more sense because I don't see where we would be going with adding to these to `to_numeric`, though that would be more inconvenient if there was a large table of small integers.

@gfyoung you could simply add them as options to `to_numeric`, which is basically a smart `.astype`.
(that handles errors and such).

How would they be toggled from the parser API exactly?

@gfyoung they wouldn't that why we deprecate :>

though you _could_ very easily. Move these to a post-processing step and simply call something like

```
if compact_ints:
    x = pd.to_numeric(x, errors='ignore', compact_ints=compact_ints, use_unsigned=use_unsigned)
```

and simply move the code `downcast_int64` to a call inside `pd.to_numeric`. (and move the code to inference.pyx just for cleanliness).

This is indepedent of deprecation or not though. (e.g. you can fix this and can separately deprecate).

Ah, I see.  Will do that then.

Actually, it's not as straightforward as it seems.  `parser.pyx` creates its own list of `na_values` (see <a href="https://github.com/pydata/pandas/blob/da5fc17fe9bee7527fb89b4540dc0c40555aaf16/pandas/parser.pyx#L1873">here</a>) that it uses when casting inside `_downcast_to_int64`, and that list of values is used in other functions within the file.  If I move it to `inference.pyx` to be called via `to_numeric` (which I'm no longer sure about since that is a further API change), I would have to duplicate the code that creates `na_values` - how does the refactoring sound now?

you can just do this is maybe_convert_numeric or equivalent 

@jreback : Refactored `downcast_to_int64` as a function in `inference.pyx`, and Travis is happy.  So just waiting to see what @jorisvandenbossche has to say about deprecation.

I don't think this works if the arr has any na_values in it
(not tested either)

@jreback : I literally moved the function from one location to another.  If that's an issue, then it would have appeared in the parser as well.  Will add a test though for that.

@gfyoung no, if its by efinition int64 coming in, then `na_values` are superfulous here. I know you copied it. that's not to say it was completely correct before.

@jreback : `na_values` is actually used in several places in the implementation, so it's not as superfluous as you make it sound (unless you would like to hard-code values).  Also, the function does work when the `na_value` for `np.int64` is included in the array.  Added test to make sure.

@gfyoung you are missing my point. int arrays don't support missing values (currently), so if you actually have a missing value then this must be converted to float which is impossible in the current impl. so they ARE superfluous.

@jreback : That's not the point of this function at all.  If it has missing values (i.e.`nan`), the array won't go through this path.  That `na_values` parameter is hardly superfluous if you look at what it actually has stored in it.

@gfyoung then prove it by showing an example

@jreback : I think my test cases speak for themselves.

they don't prove anything, you aren't actually using `na_values`

@jreback : really?  did you look at my last test case?

yes, we don't support intNA markers. So its a wrong case.

that's the issue here. This is a completely incorrect function. You can only compact ints if you HAVE INTS. By definition if you have a NaN value then it needs to be upcast. 

I don't really follow you here.  What's the problem with only being able to compact ints when you have ints, and what do you mean by "upcasting `NaN`"? (yes, I did already look at `_maybe_upcast` in `parser.pyx`)

we don't support sentinels for ints, that's what this does. So its _very_ misleading. Its like you can say hey this int is a NaN value, so mark it (but oh, keep this as ints). We don't support this at all.

Let's just deprecate this (documenting is ok I guess). This is really broken.

Ah, I see.  Now I understand.  Okay, so KEEP the documentation changes but UNDO the refactoring and added Python support, right?

well you can keep it. Because I think this _should_ actually go in `to_numeric`, eg. from a user perspective I _do_ want to `compact_ints` (probably by default if I am calling `to_numeric`, BUT If I have nans then of course we need to leave as floats.

Let's open a new issue for this, enhancement to `to_numeric` (another would be to pass in a list of `na_values`), IOW, values that we allow as going to `nan`, but will work even with `errors='raise'` (e.g. its not an error but a nan).

Fair enough.  I presume that the `to_numeric` enhancement would leave out the sentinels or not?

yes I think u can compact (if it's already int) otherwise could convert (to numeric) then compact

it's kind of like a fancy astype