Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Nov 26, 2018
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.24.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ Other Enhancements
- Compatibility with Matplotlib 3.0 (:issue:`22790`).
- Added :meth:`Interval.overlaps`, :meth:`IntervalArray.overlaps`, and :meth:`IntervalIndex.overlaps` for determining overlaps between interval-like objects (:issue:`21998`)
- :meth:`Timestamp.tz_localize`, :meth:`DatetimeIndex.tz_localize`, and :meth:`Series.tz_localize` have gained the ``nonexistent`` argument for alternative handling of nonexistent times. See :ref:`timeseries.timezone_nonexsistent` (:issue:`8917`)
- :func:`read_fwf` now accepts keyword `infer_nrows` (:issue:`15138`).

.. _whatsnew_0240.api_breaking:

Expand Down
37 changes: 27 additions & 10 deletions pandas/io/parsers.py
Original file line number Diff line number Diff line change
Expand Up @@ -343,15 +343,23 @@
_engine_doc))

_fwf_widths = """\
colspecs : list of pairs (int, int) or 'infer'. optional
colspecs : list of pairs (int, int) or 'infer', default 'infer'
A list of pairs (tuples) giving the extents of the fixed-width
fields of each line as half-open intervals (i.e., [from, to[ ).
fields of each line as half-open intervals (i.e., [from, to) ).
String value 'infer' can be used to instruct the parser to try
detecting the column specifications from the first 100 rows of
the data which are not being skipped via skiprows (default='infer').
widths : list of ints. optional
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.
widths : list of ints, optional
A list of field widths which can be used instead of `colspecs` if
the intervals are contiguous.
infer_nrows : int, default None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default 100

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right but better to simply say its 100

The number of rows to consider when letting the parser determine the
``colspecs``. If not set (or set to `None`), default behavior of 100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put literals like None in double backpacks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

rows is used.

.. versionadded:: 0.24.0

delimiter : str, default ``'\t' + ' '``
Characters to consider as filler characters in the fixed-width file.
Can be used to specify the filler character of the fields
Expand Down Expand Up @@ -528,6 +536,7 @@ def _read(filepath_or_buffer, kwds):

_fwf_defaults = {
'colspecs': 'infer',
'infer_nrows': None,
'widths': None,
}

Expand Down Expand Up @@ -717,7 +726,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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

# Check input arguments.
if colspecs is None and widths is None:
raise ValueError("Must specify either colspecs or widths")
Expand All @@ -733,6 +743,7 @@ def read_fwf(filepath_or_buffer, colspecs='infer', widths=None, **kwds):
col += w

kwds['colspecs'] = colspecs
kwds['infer_nrows'] = infer_nrows
kwds['engine'] = 'python-fwf'
return _read(filepath_or_buffer, kwds)

Expand Down Expand Up @@ -3363,13 +3374,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, skiprows=None,
infer_nrows=None):
self.f = f
self.buffer = None
self.delimiter = '\r\n' + delimiter if delimiter else '\n\r\t '
self.comment = comment
if colspecs == 'infer':
self.colspecs = self.detect_colspecs(skiprows=skiprows)
self.colspecs = self.detect_colspecs(skiprows=skiprows,
infer_nrows=infer_nrows)
else:
self.colspecs = colspecs

Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Regex escape the delimiters
delimiters = ''.join(r'\%s' % x for x in self.delimiter)
pattern = re.compile('([^%s]+)' % delimiters)
if infer_nrows:
n = infer_nrows
rows = self.get_rows(n, skiprows)
if not rows:
raise EmptyDataError("No rows from which to infer column width")
Expand Down Expand Up @@ -3465,8 +3480,10 @@ class FixedWidthFieldParser(PythonParser):
def __init__(self, f, **kwds):
# Support iterators, convert to a list.
self.colspecs = kwds.pop('colspecs')
self.infer_nrows = kwds.pop('infer_nrows')
PythonParser.__init__(self, f, **kwds)

def _make_reader(self, f):
self.data = FixedWidthReader(f, self.colspecs, self.delimiter,
self.comment, self.skiprows)
self.comment, self.skiprows,
self.infer_nrows)
16 changes: 16 additions & 0 deletions pandas/tests/io/parser/test_read_fwf.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,22 @@ def test_fwf_colspecs_None(self):
expected = DataFrame([[123456, 456], [456789, 789]])
tm.assert_frame_equal(result, expected)

def test_fwf_colspecs_infer_nrows(self):
# GH 15138
data = """\
1 2
123 98
"""
# infer_nrows == 1 should have colspec == [(2, 3), (5, 6)]
df = read_fwf(StringIO(data), header=None, infer_nrows=1)
expected = pd.DataFrame([[1, 2], [3, 8]])
tm.assert_frame_equal(df, expected)

# test for infer_nrows > number of rows
df = read_fwf(StringIO(data), header=None, infer_nrows=10)
expected = pd.DataFrame([[1, 2], [123, 98]])
tm.assert_frame_equal(df, expected)

def test_fwf_regression(self):
# GH 3594
# turns out 'T060' is parsable as a datetime slice!
Expand Down