BUG: provide chunks with progressively numbered (default) indices #12289
Conversation
| new_rows = len(col_dict[columns[0]]) | ||
| index = RangeIndex(self._currow, self._currow + new_rows) | ||
| else: | ||
| new_rows = len(index) |
There was a problem hiding this comment.
use new_rows = len(col_dict.values()[0]) (this can be outside the if) as I think this might fail with duplicate column names (add a test for that as well)
you don't need the else either
There was a problem hiding this comment.
OK for col_dict.values(), but I think we want to consider valid csv files with only index, so the if...else is needed.
There was a problem hiding this comment.
Oh, and by the way... isn't col_dict a dict?! Anyway I will add a test.
There was a problem hiding this comment.
you still need the if, my point is that you only need to do this if its None
There was a problem hiding this comment.
OK, I see. My point then was that having a valid self.currow (updated just below) is an added value per se. For instance it is few lines of code away from enabling chunksize together with nrows.
|
needs to use the parser data directly. If not available, then needs to be exposed. This makes it less complicated from a future reader perspective. |
|
can you rebase/update |
|
can you rebase / update |
|
can you rebase and i'll have a look |
| index = RangeIndex(self._currow, self._currow + new_rows) | ||
| else: | ||
| new_rows = 0 | ||
| index = Index([]) |
There was a problem hiding this comment.
An (empty) RangeIndex here breaks some comparisons with the output of the CParser with low_memory=True. Both should probably be fixed, but I'm not sure whether we need to first make RangeIndex(a, b).append(RangeIndex(b, c)) return a RangeIndex(a, c) (rather than an Int64Index, as happens currently).
There was a problem hiding this comment.
you would have to special case the logic, but not s bad idea
There was a problem hiding this comment.
don't specify the index here, let the pandas object creation logic do it.
|
Rebased. Certainly two problems left (the one you pointed out - we should already use the available info inside the parser - and the one concerning the empty chunk I commented above): I can take care of them but I don't know when, so if you want the fix in 0.19.0 I suggest to postpone them to future PRs. |
Current coverage is 85.23% (diff: 100%)@@ master #12289 diff @@
==========================================
Files 140 140
Lines 50415 50423 +8
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 42970 42977 +7
- Misses 7445 7446 +1
Partials 0 0
|
| def test_read_chunksize_generated_index(self): | ||
| # GH 12185 | ||
| reader = self.read_csv(StringIO(self.data1), chunksize=2) | ||
| df = self.read_csv(StringIO(self.data1)) |
There was a problem hiding this comment.
check with an index_col as well
There was a problem hiding this comment.
See test_read_chunksize and test_read_chunksize_named (hence the "generated_index" in the new test's name)... am I missing something?
There was a problem hiding this comment.
this need some more comprehensive tests. they should be in this same test method (even if they are slightly duplicated elsewhere). you are making a major change, so need to exercise multiple cases.
toobaz
force-pushed
the
csvstate
branch
2 times, most recently
from
5b65dd1 to
2677e25
Compare
|
OK, test on |
| @@ -289,6 +289,10 @@ Other enhancements | |||
|
|
|||
| pd.Timestamp(year=2012, month=1, day=1, hour=8, minute=30) | |||
|
|
|||
|
|
|||
| - ``pd.read_csv()`` with the ``chunksize=`` option and implicit index now returns an index progressively numbered, rather than in repeated chunks (:issue:`12185`) | |||
There was a problem hiding this comment.
show an example of the before and the after, this is a major change. put it in a separate sub-section.
|
Hope the sub-section is fine... I was unable to generate the docs (probably my |
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| When :func:`read_csv` is called with ``chunksize='n'`` and without specifying an index, | ||
| each chunk used to have an independently generated index from `0`` to ``n``. |
There was a problem hiding this comment.
to be fully correct this would be 'n-1' I think?
|
@toobaz The whatsnew explanation looks good I think! But can you move it a bit below to the API changes section? (it can be a subsection of that) |
|
@toobaz Can you also take a look in the io.rst docs on the explanation about chunksize to see if there is something that should/could be changed? (edit: took a quick look in the io-chunking section http://pandas.pydata.org/pandas-docs/stable/io.html#io-chunking, and I think this is good) |
|
@jorisvandenbossche : thanks for the comments, should have fixed everything. I also checked |
|
Ahem... please kill https://travis-ci.org/pydata/pandas/builds/147442885 ... |
|
@jreback this looks good to me |
|
thanks @toobaz |
closes #12185
Notice the test I fix was indeed wrong - I had written that line as a workaround waiting for this fix.