Fix #25099 set na_rep values before converting to string to prevent data truncation
State: closed

Hi,

The code in this pull request fixes #25099, and all tests pass. However, I am not sure if this is the best (or even an elegant) solution.

The issue happened because the float `NaN` is converted to `str` (i.e. "nan"), which gets stored as a `<U3`. When the user-provided `na_rep` is used, it is first truncated down to the data type length (i.e. "myn").

I tried moving the `na_rep` to before it is converted to string, but other tests fail. I could not find other code using a similar approach, of concatenating `<U` with the desired length. So I assume it's an easy hack, but not a proper solution?

Any suggestions? I also tried setting the `na_rep` value only in the `if` branch that calls `values.astype(str)`, but there are still test failures.

Cheers
Bruno

- [x] closes #25099
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Bug
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Output-Formatting
description: __repr__ of pandas objects, to_string
name: IO CSV
description: read_csv, to_csv
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=h1) Report
> Merging [#25103](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f75a220ff1e5e027ef2b070430fd7f4490cdcbf0?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25103/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25103      +/-   ##
==========================================
+ Coverage   92.37%   92.37%   +<.01%     
==========================================
  Files         166      166              
  Lines       52415    52417       +2     
==========================================
+ Hits        48418    48420       +2     
  Misses       3997     3997
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.79% <100%> (ø)` | :arrow_up: |
| #single | `42.87% <66.66%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/25103/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `94.18% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=footer). Last update [f75a220...f393d18](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@e0c63b4`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25103/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master   #25103   +/-   ##
=========================================
  Coverage          ?   40.71%           
=========================================
  Files             ?      175           
  Lines             ?    52378           
  Branches          ?        0           
=========================================
  Hits              ?    21328           
  Misses            ?    31050           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `40.71% <100%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/25103/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `51.2% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=footer). Last update [e0c63b4...10f67ab](https://codecov.io/gh/pandas-dev/pandas/pull/25103?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Hello @kinow! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:





There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-09-10 00:01:49 UTC
@jreback just finished running asv with the following command

```bash
asv continuous -f 1.1 upstream/master HEAD -b ^io.csv
```

Complete output in this [gist](https://gist.github.com/kinow/9e3760ddfa036b82df0f587323e17ee6), but near the bottom of its output, it says

```
+      9.92±0.1ms       11.7±0.6ms     1.18  io.csv.ReadCSVFloatPrecision.time_read_csv_python_engine(',', '_', None)
+         119±1ms          135±4ms     1.14  io.csv.ReadCSVCategorical.time_convert_direct
+         171±5ms          190±4ms     1.11  io.csv.ReadCSVCategorical.time_convert_post

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
```

Which doesn't look good I think... do you reckon I should try to find another alternative, perhaps duplicating code and leaving the previous method unaltered? 

By the way, I didn't find a `pytables.py` under the benchmarks folder. Is there another way to trigger benchmarking tests for pytest in Pandas?

Thanks
Bruno
can you merge master
Branch rebased onto `master`
@jreback added a whatsnew entry as requested - happy to amend if text is incorrect or too simple.

I resolved the outdated discussions, except for one that is still pertinent. The initial approach was to calculate the maximum length when it was a string, and then just create a type of that length. That way the masked values were not truncated.

The pending discussion is around that approach, where we tried instead to change the order of where the mask gets applied. But several other tests failed after that (see my last comment that Travis failed). In one test that I found, it had an empty string, and for some reason with the different order, it was trying to call `int('')` and returning `ValueError: invalid literal for int() with base 10: ''`.

So I reverted that change, and Travis stopped complaining. Also ran `asv`:

```
$ asv continuous -f 1.1 upstream/master HEAD -b ^io.csv
· No information stored about machine 'ranma'. I know about nothing.
  

I will now ask you some questions about this machine to identify it in the benchmarks.

1. machine:  A unique name to identify this machine in the results.
   May be anything, as long as it is unique across all the machines
   used to benchmark this project.  NOTE: If changed from the default,
   it will no longer match the hostname of this machine, and you may
   need to explicitly use the --machine argument to asv.
machine [ranma]: 
2. os:  The OS type and version of this machine.  For example,
   'Macintosh OS-X 10.8'.
os [Linux 4.15.0-46-generic]: 
3. arch:  The generic CPU architecture of this machine.  For example,
   'i386' or 'x86_64'.
arch [x86_64]: 
4. cpu:  A specific description of the CPU of this machine, including
   its speed and class.  For example, 'Intel(R) Core(TM) i5-2520M CPU
   @ 2.50GHz (4 cores)'.
cpu [Intel(R) Core(TM) i7-3720QM CPU @ 2.60GHz]: 
5. ram:  The amount of physical RAM on this machine.  For example,
   '4GB'.
ram [16119292]: 
· Creating environments......................................................................................................................................................................................................
· Discovering benchmarks
·· Uninstalling from conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
·· Building 6e0dd9cf <fix-issue-gh25099-csv-narep> for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt.........................................................
·· Installing 6e0dd9cf <fix-issue-gh25099-csv-narep> into conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt..
· Running 32 total benchmarks (2 commits * 1 environments * 16 benchmarks)
[  0.00%] · For pandas commit 46639512 <master> (round 1/2):
[  0.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt............................................................
[  0.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[  1.56%] ··· Running (io.csv.ReadCSVCategorical.time_convert_direct--)...............
[ 25.00%] · For pandas commit 6e0dd9cf <fix-issue-gh25099-csv-narep> (round 1/2):
[ 25.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt...
[ 25.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 26.56%] ··· Running (io.csv.ReadCSVCategorical.time_convert_direct--)...............
[ 50.00%] · For pandas commit 6e0dd9cf <fix-issue-gh25099-csv-narep> (round 2/2):
[ 50.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 51.56%] ··· io.csv.ReadCSVCategorical.time_convert_direct                                                                                                          56.9±0.6ms
[ 53.12%] ··· io.csv.ReadCSVCategorical.time_convert_post                                                                                                            82.8±0.5ms
[ 54.69%] ··· io.csv.ReadCSVComment.time_comment                                                                                                                     38.6±0.6ms
[ 56.25%] ··· io.csv.ReadCSVDInferDatetimeFormat.time_read_csv                                                                                                               ok
[ 56.25%] ··· ======================= ============= ============= =============
              --                                        format                 
              ----------------------- -----------------------------------------
               infer_datetime_format      custom       iso8601         ymd     
              ======================= ============= ============= =============
                        True           8.42±0.02ms   2.87±0.03ms   2.89±0.01ms 
                       False             138±1ms     2.41±0.01ms   2.27±0.01ms 
              ======================= ============= ============= =============

[ 57.81%] ··· io.csv.ReadCSVFloatPrecision.time_read_csv                                                                                                                     ok
[ 57.81%] ··· ===== ============= ============= ================ ============= ============= ================
              --                                    decimal / float_precision                                
              ----- -----------------------------------------------------------------------------------------
               sep     . / None      . / high    . / round_trip     _ / None      _ / high    _ / round_trip 
              ===== ============= ============= ================ ============= ============= ================
                ,    2.31±0.02ms   2.21±0.04ms    2.95±0.03ms     2.56±0.03ms   2.56±0.02ms    2.56±0.03ms   
                ;    2.33±0.02ms   2.24±0.05ms    2.96±0.04ms     2.53±0.03ms   2.49±0.04ms    2.53±0.03ms   
              ===== ============= ============= ================ ============= ============= ================

[ 59.38%] ··· io.csv.ReadCSVFloatPrecision.time_read_csv_python_engine                                                                                                       ok
[ 59.38%] ··· ===== ============ ============ ================ ============= ============= ================
              --                                   decimal / float_precision                               
              ----- ---------------------------------------------------------------------------------------
               sep    . / None     . / high    . / round_trip     _ / None      _ / high    _ / round_trip 
              ===== ============ ============ ================ ============= ============= ================
                ,    4.66±0.2ms   4.63±0.1ms     4.70±0.1ms     3.90±0.05ms   3.90±0.03ms    3.89±0.05ms   
                ;    4.64±0.2ms   4.62±0.1ms     4.62±0.2ms     3.88±0.02ms   3.89±0.05ms    3.90±0.04ms   
              ===== ============ ============ ================ ============= ============= ================

[ 60.94%] ··· io.csv.ReadCSVMemoryGrowth.mem_parser_chunks                                                                                                                    0
[ 62.50%] ··· io.csv.ReadCSVParseDates.time_baseline                                                                                                                2.71±0.05ms
[ 64.06%] ··· io.csv.ReadCSVParseDates.time_multiple_date                                                                                                           3.13±0.01ms
[ 65.62%] ··· io.csv.ReadCSVSkipRows.time_skipprows                                                                                                                          ok
[ 65.62%] ··· ========== ============
               skiprows              
              ---------- ------------
                 None     23.0±0.2ms 
                10000     15.1±0.3ms 
              ========== ============

[ 67.19%] ··· io.csv.ReadCSVThousands.time_thousands                                                                                                                         ok
[ 67.19%] ··· ===== ============ ============
              --            thousands        
              ----- -------------------------
               sep      None          ,      
              ===== ============ ============
                ,    18.6±0.2ms   19.5±0.5ms 
                |    18.6±0.3ms   20.7±0.2ms 
              ===== ============ ============

[ 68.75%] ··· io.csv.ReadUint64Integers.time_read_uint64                                                                                                             4.79±0.1ms
[ 70.31%] ··· io.csv.ReadUint64Integers.time_read_uint64_na_values                                                                                                  7.47±0.07ms
[ 71.88%] ··· io.csv.ReadUint64Integers.time_read_uint64_neg_values                                                                                                 7.32±0.09ms
[ 73.44%] ··· io.csv.ToCSV.time_frame                                                                                                                                        ok
[ 73.44%] ··· ======= ============
                kind              
              ------- ------------
                wide   133±0.8ms  
                long    237±1ms   
               mixed   29.8±0.6ms 
              ======= ============

[ 75.00%] ··· io.csv.ToCSVDatetime.time_frame_date_formatting                                                                                                        25.5±0.2ms
[ 75.00%] · For pandas commit 46639512 <master> (round 2/2):
[ 75.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt...
[ 75.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 76.56%] ··· io.csv.ReadCSVCategorical.time_convert_direct                                                                                                          53.8±0.3ms
[ 78.12%] ··· io.csv.ReadCSVCategorical.time_convert_post                                                                                                            80.2±0.5ms
[ 79.69%] ··· io.csv.ReadCSVComment.time_comment                                                                                                                     39.6±0.7ms
[ 81.25%] ··· io.csv.ReadCSVDInferDatetimeFormat.time_read_csv                                                                                                               ok
[ 81.25%] ··· ======================= ============= ============= =============
              --                                        format                 
              ----------------------- -----------------------------------------
               infer_datetime_format      custom       iso8601         ymd     
              ======================= ============= ============= =============
                        True           8.48±0.06ms   2.89±0.02ms   2.87±0.03ms 
                       False            140±0.3ms    2.42±0.01ms   2.31±0.01ms 
              ======================= ============= ============= =============

[ 82.81%] ··· io.csv.ReadCSVFloatPrecision.time_read_csv                                                                                                                     ok
[ 82.81%] ··· ===== ============= ============= ================ ============= ============= ================
              --                                    decimal / float_precision                                
              ----- -----------------------------------------------------------------------------------------
               sep     . / None      . / high    . / round_trip     _ / None      _ / high    _ / round_trip 
              ===== ============= ============= ================ ============= ============= ================
                ,    2.31±0.04ms   2.22±0.03ms    3.00±0.04ms     2.58±0.04ms   2.58±0.04ms    2.63±0.05ms   
                ;    2.32±0.05ms   2.21±0.04ms    3.00±0.04ms     2.62±0.04ms   2.63±0.03ms    2.59±0.04ms   
              ===== ============= ============= ================ ============= ============= ================

[ 84.38%] ··· io.csv.ReadCSVFloatPrecision.time_read_csv_python_engine                                                                                                       ok
[ 84.38%] ··· ===== ============ ============ ================ ============ ============ ================
              --                                  decimal / float_precision                              
              ----- -------------------------------------------------------------------------------------
               sep    . / None     . / high    . / round_trip    _ / None     _ / high    _ / round_trip 
              ===== ============ ============ ================ ============ ============ ================
                ,    5.10±0.6ms   5.07±0.6ms     5.06±0.5ms     4.43±0.5ms   4.36±0.5ms     4.48±0.6ms   
                ;    5.07±0.6ms   5.05±0.5ms     5.09±0.5ms     4.41±0.5ms   4.36±0.5ms     4.41±0.6ms   
              ===== ============ ============ ================ ============ ============ ================

[ 85.94%] ··· io.csv.ReadCSVMemoryGrowth.mem_parser_chunks                                                                                                                    0
[ 87.50%] ··· io.csv.ReadCSVParseDates.time_baseline                                                                                                                2.72±0.05ms
[ 89.06%] ··· io.csv.ReadCSVParseDates.time_multiple_date                                                                                                           3.13±0.02ms
[ 90.62%] ··· io.csv.ReadCSVSkipRows.time_skipprows                                                                                                                          ok
[ 90.62%] ··· ========== ============
               skiprows              
              ---------- ------------
                 None     22.9±0.3ms 
                10000     15.0±0.1ms 
              ========== ============

[ 92.19%] ··· io.csv.ReadCSVThousands.time_thousands                                                                                                                         ok
[ 92.19%] ··· ===== ============ ============
              --            thousands        
              ----- -------------------------
               sep      None          ,      
              ===== ============ ============
                ,    18.9±0.2ms   19.6±0.2ms 
                |    19.1±0.2ms   20.6±0.1ms 
              ===== ============ ============

[ 93.75%] ··· io.csv.ReadUint64Integers.time_read_uint64                                                                                                            4.75±0.06ms
[ 95.31%] ··· io.csv.ReadUint64Integers.time_read_uint64_na_values                                                                                                  7.38±0.08ms
[ 96.88%] ··· io.csv.ReadUint64Integers.time_read_uint64_neg_values                                                                                                  7.32±0.1ms
[ 98.44%] ··· io.csv.ToCSV.time_frame                                                                                                                                        ok
[ 98.44%] ··· ======= ============
                kind              
              ------- ------------
                wide   130±0.4ms  
                long    234±5ms   
               mixed   28.9±0.4ms 
              ======= ============

[100.00%] ··· io.csv.ToCSVDatetime.time_frame_date_formatting                                                                                                        25.7±0.5ms

BENCHMARKS NOT SIGNIFICANTLY CHANGED.
```

However, I am still not too confident if that is a valid approach, as I did not find other parts of the code in Pandas where types had a dynamic size :confused: 

I hope that makes sense.
Thanks for the review @WillAyd 

>Can you merge master?

Done. Rebased, fixed conflicts, and tried to address the feedback, except the parameterized test as that was requested before (I think the intention was to add more test scenarios).

Cheers
Bruno
can you merge master
@kinow can you merge master.

Conflicting files
doc/source/whatsnew/v0.25.0.rst
pandas/core/internals/blocks.py
Done @simonjayhawkins :+1: 
this looked reasonble, can you merge master
Done @jreback 👍 
Hi @WillAyd 

>Looks like this went for a bit without review - @kinow any chance this is still active?

Sure thing, will just need to re-read last comments and rebase the code. Thanks for the review, I will try to update it today or over the weekend 👍 
Rebased, now will try to find some time this weekend to troubleshoot the Travis errors, and see if I can fix them and them look at `PyUnicode_GET_LENGTH` too.
@WillAyd rebased, fixed conflicts, tried to address your feedback the best I could, and squashed commits too. CI passing, so should be ready for review again 👍 thanks
>lgtm. By the way @kinow no need to squash your commits we do so before merge

Oh, thanks for the heads-up, I thought I would be asked to squash them later. And I realized I had made a mess in the last commits, so decided to tidy it up a bit. Lesson learned for the next PR. Thanks!
Looks like we have a couple +1s. Thanks @kinow!