FIX-#3417: Fix read_csv with skiprows and header parameters
#3419
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amyskov
force-pushed
the
fix_read_csv_issue
branch
from
2e4762b
to
affc60e
Compare
Codecov Report
@@ Coverage Diff @@
## master #3419 +/- ##
===========================================
- Coverage 85.61% 48.98% -36.63%
===========================================
Files 201 187 -14
Lines 16852 16034 -818
===========================================
- Hits 14428 7855 -6573
- Misses 2424 8179 +5755
Continue to review full report at Codecov.
|
amyskov
force-pushed
the
fix_read_csv_issue
branch
from
d970de1
to
0b524c8
Compare
|
|
|
|
PR is ready for review. Changes performance measurements (no performance degradation was obtained): |
|
@amyskov if we need so much code for each edge case, then the code base for IO functions will be very difficult to maintain. |
You are right, such edge cases bring logic complication. These edge cases occurs because of complex interconnections of |
|
What if we delete rows after the dataframe is created? |
We can do that, but we still should know what to skip. See pandas example below: Output: In this case rows |
|
Hi @modin-project/modin-core please take a look at this PR. |
| # when `skiprows_md` contains `in` operator | ||
| mod_index = skiprows_md(index_range) | ||
| assert is_list_like(mod_index) | ||
| except Exception: |
There was a problem hiding this comment.
Why catch all exception types? This is not ideal
There was a problem hiding this comment.
Ok, exception types were clarified.
|
|
||
| Returns | ||
| ------- | ||
| skiprows : int, array or callable | ||
| Updated skiprows parameter. | ||
| skiprows_md : int, array or callable |
There was a problem hiding this comment.
md stands for Modin. In the context of this and csv_dispatcher._read functions skiprows is original pandas.read_csv skiprows parameter, while skiprows_md is parameter version modified for Modin specific purposes (sorting of list-like parameter, etc.)
7 tasks
amyskov
force-pushed
the
fix_read_csv_issue
branch
from
0b524c8
to
df35302
Compare
There was a problem hiding this comment.
I tend to agree with @anmyachev about the complexity of the code.
pandas read_csv has a lot of edge cases and parameter combinations. It would be good to try to solve more generally or in a more compact and maintainable way.
I know this is blocking other issues, I am ok to proceed with this one, but adding 100+ lines for each combination of parameters will make the code unmaintainable.
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
anmyachev
reviewed
Dec 9, 2021
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
It was done in attempt to localize all manipulations with |
anmyachev
reviewed
Dec 10, 2021
anmyachev
reviewed
Dec 10, 2021
Comment on lines
653
to
655
| is_list_like(skiprows_md) and skiprows_md[0] < header_size | ||
| ) or ( | ||
| callable(skiprows) |
There was a problem hiding this comment.
Why are you use skiprows and skiprowd_md variables at the same time?
There was a problem hiding this comment.
This is done in that way because in the case of list-like skiprows we need already sorted array to get the smallest element (the first element in the skiprows_md array) but in the case of callable skiprows we can't get skiprows_md since it's value already shifted on header_size value and we can't get intersection information from it (so we need unshifted skiprows callable).
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
amyskov
force-pushed
the
fix_read_csv_issue
branch
from
6bc8cbd
to
a054a26
Compare
anmyachev
reviewed
Dec 13, 2021
anmyachev
previously approved these changes
Dec 13, 2021
…r.py Co-authored-by: Anatoly Myachev <anatoly.myachev@intel.com> Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
amyskov
force-pushed
the
fix_read_csv_issue
branch
from
8623bc7
to
c04de5b
Compare
|
@anmyachev @devin-petersohn do you have some additional comments? |
anmyachev
previously approved these changes
Dec 16, 2021
|
@amyskov, I think we can merge this PR after @devin-petersohn's comments are addressed and conflicts are resolved. |
Co-authored-by: Devin Petersohn <devin-petersohn@users.noreply.github.com>
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
|
@amyskov, please update release notes. |
Signed-off-by: Alexander Myskov <alexander.myskov@intel.com>
Right. Release notes doc was updated. |
YarShev
approved these changes
Feb 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Alexander Myskov alexander.myskov@intel.com
What do these changes do?
PR fixes corner cases of
read_csvfunction withskiprowsandheaderparameters, which values have intersections. Also manipulations withskiprowsparameter were localized in_manage_skiprows_parameterfunction.flake8 modinblack --check modingit commit -sread_csvfails withskiprowsandheaderparameters passed #3417docs/developer/architecture.rstis up-to-date