ENH: support "nrows" and "chunksize" together #15756
Conversation
jreback left a comment
jreback left a comment There was a problem hiding this comment.
minor comment. lgtm.
can you also add some tests for nrows/chunksize that they must be positive integers?
doc/source/whatsnew/v0.20.0.txt Outdated
| - ``Series`` provides a ``to_excel`` method to output Excel files (:issue:`8825`) | ||
| - The ``usecols`` argument in ``pd.read_csv`` now accepts a callable function as a value (:issue:`14154`) | ||
| - The ``skiprows`` argument in ``pd.read_csv`` now accepts a callable function as a value (:issue:`10882`) | ||
| - The ``nrows`` and ``chunksize`` arguments in ``pd.read_csv`` are no more incompatible (:issue:`15755`) |
There was a problem hiding this comment.
no more incompatible -> are supported if both are passed.
| actually I think we need some tests if chunksize > nrows. and with get_chunk with nrows & chunksize specified. (mostly what happens at the edge point). |
Codecov Report
@@ Coverage Diff @@ ## master #15756 +/- ## ========================================== - Coverage 91.01% 91% -0.02% ========================================== Files 143 143 Lines 49371 49370 -1 ========================================== - Hits 44937 44928 -9 - Misses 4434 4442 +8
Continue to review full report at Codecov.
|
shall we cast floats (current behavior, as of #10476), or raise? |
nrows already has a validation function (which casts to float if possible), we can probably do the same for chunksize |
| There is currently actually a memory leak/infinite loop when doing eg Negative values for nrows/chunksize currently return an empty frame, so which is kind of valid. If we want to change that, I would keep that for another PR/issue. |
yep that's a good idea |
Agree, but since chunksize=0 easily results, as @jorisvandenbossche mentioned, in infinite loops, I would impose chunksize >= 1. And maybe for (partial - asking nrows=0 is fine) coherence we want to impose nrows >= 0? (I would like this, because negative chunksize/nrows could be misinterpreted as parsing starting from the end...) In the meanwhile, I have pushed the requested tests for the original edit, in case we want to move the rest/discussion to another PR |
I think this is kind of wonky. We don't actually support indexing from the end of the file. I would raise on this. (can do in another independent issue). |
| if size is None: | ||
| size = self.chunksize | ||
| if self.nrows is not None: | ||
| if self._currow >= self.nrows: |
There was a problem hiding this comment.
why isn't this just
if self.nrows is not None: if self._currow >= self.nrows: size = 0 size = min(....) return self.read(...) ?
There was a problem hiding this comment.
raising StopIteration directly here seems odd. as self.read(nrows=0) correctly returns the result (as an empty DataFrame)
There was a problem hiding this comment.
Because a parser's .read() raises (or at least, can legitimately raise) StopIteration only when the current row increases above self.nrows, and this never happens if you keep asking for 0 rows at a time. This is not hypothetical - the "# With nrows" step of test_read_chunksize in pandas/tests/io/parser/common.py hangs for the PythonParser (and possibly others) if I change the code as you suggest.
| cc @gfyoung if you can have a look. |
| LGTM! |
| thanks @toobaz |
| You're welcome! |
closes pandas-dev#15755 Author: Pietro Battiston <me@pietrobattiston.it> Closes pandas-dev#15756 from toobaz/nrows_chunksize and squashes the following commits: d0288e3 [Pietro Battiston] ENH: support "nrows" and "chunksize" together
4 participants
git diff master | flake8 --diff(except forwhatsnew/v0.20.0.txt, I don't know why)