ENH/TST/DOC: set infer_nrows for read_fwf (GH15138) #23238

rdmontgomery · 2018-10-19T14:07:58Z

closes read_fwf 'infer' where first hundred lines differ from other lines #15138
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

read_fwf can infer the widths of columns of a dataset from a file or buffer, but if the widths change after the first 100 rows, then the default behavior fails to capture the data correctly. This PR addresses that issue by allowing the user to set a parameter infer_nrows that is passed down to the appropriate function.

The associated test ensures that colspecs can be determined from the first row, thereby missing data from the subsequent rows and verifying the expected behavior.

Previously, read_fwf would infer the width of the columns from the first 100 rows of data. It now accepts a parameter infer_nrows that lets the user choose how many rows to use in the inference.

Fixed reference in colspecs. Switched half-open interval representation from non-standard [from, to[ to standard [from, to).

pep8speaks · 2018-10-19T14:08:01Z

Hello @rdmontgomery! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/io/parsers.py !
There are no PEP8 issues in the file pandas/tests/io/parser/test_read_fwf.py !

WillAyd · 2018-10-23T05:06:05Z

pandas/io/parsers.py

@@ -716,7 +720,8 @@ def parser_f(filepath_or_buffer,


 @Appender(_read_fwf_doc)
-def read_fwf(filepath_or_buffer, colspecs='infer', widths=None, **kwds):
+def read_fwf(filepath_or_buffer, colspecs='infer', infer_nrows=100,


Should add the new keyword to the end of positional arguments so as not to interfere with existing user code

Thanks, @WillAyd.

WillAyd · 2018-10-23T05:08:53Z

pandas/io/parsers.py

@@ -3420,11 +3428,11 @@ def get_rows(self, n, skiprows=None):
        self.buffer = iter(buffer_rows)
        return detect_rows

-    def detect_colspecs(self, n=100, skiprows=None):
+    def detect_colspecs(self, infer_nrows, skiprows=None):


Is there a reason why you changed this from required to optional? Think that might be causing your test failures

It was an oversight. Fixed now. Thanks!

codecov · 2018-10-23T17:59:14Z

Codecov Report

Merging #23238 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23238      +/-   ##
==========================================
+ Coverage   92.29%   92.29%   +<.01%     
==========================================
  Files         161      161              
  Lines       51498    51500       +2     
==========================================
+ Hits        47530    47533       +3     
+ Misses       3968     3967       -1

Flag	Coverage Δ
#multiple	`90.69% <100%> (ø)`	⬆️
#single	`42.42% <44.44%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/parsers.py	`95.36% <100%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7b187a...ecf35d6. Read the comment docs.

rdmontgomery · 2018-10-23T18:06:43Z

Hi @WillAyd, thanks for reviewing my code. I made the requested changes and it looks like everything passed now. Is there anything else I can do for this?

WillAyd · 2018-10-24T14:28:39Z

pandas/io/parsers.py

@@ -3361,13 +3367,15 @@ class FixedWidthReader(BaseIterator):
    A reader of fixed-width lines.
    """

-    def __init__(self, f, colspecs, delimiter, comment, skiprows=None):
+    def __init__(self, f, colspecs, delimiter, comment, infer_nrows=100,


Still need to change position of infer_nrows to be at the end here

WillAyd · 2018-10-24T14:28:54Z

pandas/io/parsers.py

@@ -3420,11 +3428,11 @@ def get_rows(self, n, skiprows=None):
        self.buffer = iter(buffer_rows)
        return detect_rows

-    def detect_colspecs(self, n=100, skiprows=None):
+    def detect_colspecs(self, infer_nrows=100, skiprows=None):


Same as above

Oh, I actually see another problem here. I co-opted n because it was performing the intended function of infer_nrows, but that would also break previous code that relied on calling n explicitly. I'll return the code and tack on infer_nrows at the end, like you pointed out.

Rework so that if infer_nrows is not set, detect_colspecs still uses n=100, preserving previous behavior. If infer_nrows comes down the pipe, the default n is overwritten. Also make sure that the new parameter infer_nrows is the trailing keyword in all function calls. Fix the documentation to reflect these changes.

jreback

pls add a whatsnew note as well (other enhancements)

jreback · 2018-10-26T01:07:54Z

pandas/io/parsers.py

-    A list of field widths which can be used instead of 'colspecs' if
+    the data which are not being skipped via skiprows (default='infer'),
+    or by using the `infer_nrows` parameter.
+infer_nrows : int, default None


can you add infer_nrows after widths. pls add a versionadded tag

default 100

I'm switching the places and adding a versionadded tag.

Regarding the default of None, I mention my reasoning in the comment for read_fwf. I do state here in the docs
If not set (or set to `None`), default behavior of 100 rows is used.

right but better to simply say its 100

jreback · 2018-10-26T01:08:42Z

pandas/io/parsers.py

@@ -716,7 +722,8 @@ def parser_f(filepath_or_buffer,


 @Appender(_read_fwf_doc)
-def read_fwf(filepath_or_buffer, colspecs='infer', widths=None, **kwds):
+def read_fwf(filepath_or_buffer, colspecs='infer', widths=None,
+             infer_nrows=None, **kwds):


shouldn't this be 100?

I might be bending over to maintain backwards compatibility here (or I might be wrong), but I decided to let infer_nrows default to None so that if there is old code that had explicitly called detect_colspecs(n=1234), the function would behave as they expected. If someone sets infer_nrows to a number, then n is overwritten with infer_nrows.

To clarify a little further, this was the only way I could think to respect old code that explicitly set n in a direct call to detect_colspecs. Any new code that chooses to use infer_nrows will overwrite n, but I did not want to automatically overwrite n with a default of 100 if someone had already chosen to do otherwise.

this is still backward compat. If n is not None, use it, else use infer_nrows. (and let's deprecate setting n that way anyhow).

pandas/tests/io/parser/test_read_fwf.py

WillAyd · 2018-10-27T00:24:17Z

pandas/io/parsers.py

    the intervals are contiguous.
+infer_nrows : int, default None
+    The number of rows to consider when letting the parser determine the
+    ``colspecs``. If not set (or set to `None`), default behavior of 100


Put literals like None in double backpacks

Can you point me to a guide on the use of single and double backticks in pandas or GitHub? I haven't been able to find anything definitive. Or is it just single backticks for code and double backticks for literals? Should colspecs here be in singles?

WillAyd · 2018-10-27T00:27:15Z

pandas/io/parsers.py

@@ -3422,10 +3435,12 @@ def get_rows(self, n, skiprows=None):
        self.buffer = iter(buffer_rows)
        return detect_rows

-    def detect_colspecs(self, n=100, skiprows=None):
+    def detect_colspecs(self, n=100, skiprows=None, infer_nrows=None):


Is anything actually calling this with n? I believe this is an internal method so maybe here we can get by with just eliminating n altogether and replacing it with the value from infer_nrows

That's what I had proposed initially and had replaced n with infer_nrows in place. Your comment about putting the new keyword as a trailing argument is what made me realize that it was possible for someone's code to have instantiated FixedWidthReader and explicitly set n.

Granted, that seems like a rare use case, but I was able to implement a solution that handled both cases. However, this solution seems to have come at the cost of confusing documentation, where infer_nrows has a default of None in order for read_fwf(colspecs='infer') to default to inferring from the first 100 rows, as was the case beforehand.

If you and @jreback prefer, I'm happy to use either
detect_colspecs(self, infer_nrows=100, skiprows=None) or
detect_colspecs(self, skiprows=None, infer_nrows=100)
with the awareness that it might be a breaking change in rare cases.

jreback · 2018-10-28T03:37:11Z

pandas/io/parsers.py

-    A list of field widths which can be used instead of 'colspecs' if
+    the data which are not being skipped via skiprows (default='infer'),
+    or by using the `infer_nrows` parameter.
+infer_nrows : int, default None


right but better to simply say its 100

jreback · 2018-10-28T03:38:34Z

pandas/io/parsers.py

@@ -716,7 +722,8 @@ def parser_f(filepath_or_buffer,


 @Appender(_read_fwf_doc)
-def read_fwf(filepath_or_buffer, colspecs='infer', widths=None, **kwds):
+def read_fwf(filepath_or_buffer, colspecs='infer', widths=None,
+             infer_nrows=None, **kwds):


this is still backward compat. If n is not None, use it, else use infer_nrows. (and let's deprecate setting n that way anyhow).

Preserve backwards compatibility by allowing n to be set, but the new default will be to use infer_nrows.

into fwf-infer-nrows

rdmontgomery · 2018-10-28T19:46:39Z

Thanks, guys. I appreciate your help in moving this forward.

I'm not sure what happened with the CircleCI build this time around. Looks like maybe debian.org was having temporary troubles. Any advice here?

Err:3 http://deb.debian.org/debian stretch Release
404 Not Found

rdmontgomery · 2018-11-05T19:14:28Z

Hi @jreback and @WillAyd, can you help me understand why the CircleCI test failed? I'd like to re-run the test without pushing any code changes, but I haven't been able to figure out how.

jreback · 2018-11-06T01:57:21Z

looks suprious. merge master and push again.

WillAyd · 2018-11-09T05:45:34Z

pandas/io/parsers.py

        # Regex escape the delimiters
        delimiters = ''.join(r'\%s' % x for x in self.delimiter)
        pattern = re.compile('([^%s]+)' % delimiters)
-        rows = self.get_rows(n, skiprows)
+        if n:


Since this is an internal method only and not part of the api I wonder if it wouldn't be better to just replace the n keyword with infer_rows

That sounds reasonable to me, @WillAyd. Would it be better to leave the keyword order as-is with n removed or should I reorder like this def detect_colspecs(self, infer_nrows=100, skiprows=None):? Or does that really not matter at all?

I'd go with the latter unless @jreback has a differing opinion

i think ok to reorder that is an internal method

jreback · 2018-11-11T18:45:34Z

@rdmontgomery can you merge master. @WillAyd if you have any comments.

WillAyd · 2018-11-12T04:43:36Z

@rdmontgomery would be curious if you have thoughts on my previous comment. Always good to simplify / cleanup internal code if possible

jreback · 2018-11-18T18:41:43Z

pls merge master as well

jreback · 2018-11-18T22:50:05Z

rebased.

WillAyd · 2018-11-24T02:25:15Z

@rdmontgomery can you address last comment around the re-ordering of the internal function signature? Once that is done I'm OK to approve and merge

rdmontgomery · 2018-11-24T02:36:15Z

@rdmontgomery can you address last comment around the re-ordering of the internal function signature? Once that is done I'm OK to approve and merge

I'll jump on it tomorrow morning! I've been in the last negotiations of a job offer and I just wrapped that up today. I've got some time again!

Let infer_nrows replace the internal parameter n and reorder the keywords.

Fix bug introduced when master was merged back in and the function definition was changed along with the docstring.

WillAyd · 2018-11-26T00:36:06Z

Thanks @rdmontgomery !

Good luck with the job

rdmontgomery added 4 commits October 19, 2018 09:49

ENH: set infer_nrows for read_fwf (GH15138)

057eb29

Previously, read_fwf would infer the width of the columns from the first 100 rows of data. It now accepts a parameter infer_nrows that lets the user choose how many rows to use in the inference.

Add documentation for infer_nrows

9a9609d

Fixed reference in colspecs. Switched half-open interval representation from non-standard [from, to[ to standard [from, to).

Remove documentation for infer_nrows='all'

68d83c9

Add test to explicitly set infer_nrows

da081a8

WillAyd added the IO Data IO issues that don't fit into a more specific label label Oct 23, 2018

WillAyd requested changes Oct 23, 2018

View reviewed changes

rdmontgomery added 3 commits October 23, 2018 08:34

Move new keyword to end of positional arguments

3f69510

Make infer_nrows optional to retain behavior

f3e715d

Make infer_nrows optional when invoking object

1df0493

WillAyd requested changes Oct 24, 2018

View reviewed changes

jreback requested changes Oct 26, 2018

View reviewed changes

jreback added the IO CSV read_csv, to_csv label Oct 26, 2018

rdmontgomery and others added 4 commits October 26, 2018 14:18

Add a whatsnew note

d5de9d9

Add test for infer_nrows > number of rows

49d5594

Switch places and add versionadded tag

6675dac

Merge branch 'master' into fwf-infer-nrows

e1500cc

WillAyd requested changes Oct 27, 2018

View reviewed changes

jreback requested changes Oct 28, 2018

View reviewed changes

rdmontgomery added 2 commits October 28, 2018 15:01

Use infer_nrows unless n is set, default to 100

de29edf

Preserve backwards compatibility by allowing n to be set, but the new default will be to use infer_nrows.

Merge branch 'fwf-infer-nrows' of https://github.com/rdmontgomery/pandas

c4cfd9e

into fwf-infer-nrows

Merge branch 'master' into fwf-infer-nrows

0853be7

WillAyd reviewed Nov 9, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Nov 11, 2018

jreback approved these changes Nov 11, 2018

View reviewed changes

Merge branch 'master' into PR_TOOL_MERGE_PR_23238

3325262

rdmontgomery added 3 commits November 25, 2018 14:38

Merge branch 'master' into fwf-infer-nrows

d8c307e

Replace internal param with infer_nrows

cc1ce14

Let infer_nrows replace the internal parameter n and reorder the keywords.

Add keyword back to function definition

ecf35d6

Fix bug introduced when master was merged back in and the function definition was changed along with the docstring.

WillAyd approved these changes Nov 26, 2018

View reviewed changes

WillAyd merged commit 1f02bf2 into pandas-dev:master Nov 26, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

ENH/TST/DOC: set infer_nrows for read_fwf (GH15138) (pandas-dev#23238)

a4053a1

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

ENH/TST/DOC: set infer_nrows for read_fwf (GH15138) (pandas-dev#23238)

44e478f

ENH/TST/DOC: set infer_nrows for read_fwf (GH15138) #23238

ENH/TST/DOC: set infer_nrows for read_fwf (GH15138) #23238

Conversation

rdmontgomery commented Oct 19, 2018 • edited Loading

pep8speaks commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2018 • edited Loading

Codecov Report

rdmontgomery commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdmontgomery commented Oct 28, 2018

rdmontgomery commented Nov 5, 2018

jreback commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 11, 2018

WillAyd commented Nov 12, 2018

jreback commented Nov 18, 2018

jreback commented Nov 18, 2018

WillAyd commented Nov 24, 2018

rdmontgomery commented Nov 24, 2018

WillAyd commented Nov 26, 2018

rdmontgomery commented Oct 19, 2018 •

edited

Loading

codecov bot commented Oct 23, 2018 •

edited

Loading