FEAT-#1523: 'nrows' support to 'read_csv' added #1894
Merged
Conversation
This file contains hidden or 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
Codecov Report
@@ Coverage Diff @@
## master #1894 +/- ##
==========================================
- Coverage 81.80% 81.79% -0.02%
==========================================
Files 79 79
Lines 9389 9441 +52
==========================================
+ Hits 7681 7722 +41
- Misses 1708 1719 +11
Continue to review full report at Codecov.
|
|
|
|
devin-petersohn
requested changes
Aug 5, 2020
Collaborator
Author
|
@devin-petersohn, I guess it's ready for review. I've let the ability for Here is a time measurement of Data generatorimport numpy as np
import os
import pandas
RAND_LOW = -100
RAND_HIGH = 100
N = 2500000
M = 128
MULTILINE = True
TEST_FILENAME = os.path.abspath(
f"int_dataset-{N},{M},{RAND_LOW},{RAND_HIGH},{MULTILINE}.csv"
)
def generate_data_file(filename, row_n, col_n, multiline_rows=False):
data = {
f"col{i}": np.concatenate(
[
np.concatenate(
[
["some\nvery very very\nlong string\nwith many multilines"],
np.random.randint(RAND_LOW, RAND_HIGH, 9),
]
)
for _ in np.arange(row_n // 10)
]
)
if (i % 10 == 0 and multiline_rows)
else np.random.randint(RAND_LOW, RAND_HIGH, row_n)
for i in np.arange(col_n)
}
print("dict generated!")
df = pandas.DataFrame(data)
print("dataframe created!")
df.to_csv(filename)
print("csv ready!")
if __name__ == "__main__":
if not os.path.exists(TEST_FILENAME):
generate_data_file(TEST_FILENAME, N, M, MULTILINE)As you can see per row reading gives noticeable overhead in case of big files (2,500,000 rows). |
6 tasks
anmyachev
reviewed
Aug 26, 2020
anmyachev
reviewed
Aug 26, 2020
anmyachev
reviewed
Aug 26, 2020
dchigarev
force-pushed
the
issue-1523
branch
3 times, most recently
from
0a6ab80 to
21ed431
Compare
Collaborator
Author
|
@anmyachev @devin-petersohn I think, that I've addressed all your comments, please take a look |
anmyachev
requested changes
Aug 28, 2020
anmyachev
requested changes
Sep 1, 2020
Collaborator
|
@dchigarev ok, what about current performance? |
Collaborator
Author
the implementation is not significantly changed since that message if it's necessary I'll run these tests again at the latest commit |
Collaborator
Then that's enough. |
Collaborator
Author
|
@anmyachev I've rerun performance tests on the latest commit and saw some degradation between that message and current results, I'll investigate |
Collaborator
Author
|
@anmyachev I've fixed performance issue at the latest commit, the reason was that I've passed |
dchigarev
force-pushed
the
issue-1523
branch
from
df0d78c to
b805866
Compare
Collaborator
So, you did skip rows twice, why tests were be passed? |
Collaborator
Author
No, it was just triggering a slow path of reading data (via |
Collaborator
It's strange :) Ok |
dchigarev
force-pushed
the
issue-1523
branch
from
b805866 to
66a49f9
Compare
Collaborator
|
@dchigarev look at TeamCity build and rerun if needed. |
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
dchigarev
force-pushed
the
issue-1523
branch
from
66a49f9 to
b981ecf
Compare
Collaborator
Author
now all tests passes |
anmyachev
approved these changes
Sep 3, 2020
Collaborator
anmyachev
left a comment
anmyachev
left a comment
There was a problem hiding this comment.
@dchigarev great change!
Collaborator
|
@devin-petersohn it's up to you :) |
Collaborator
Author
|
@devin-petersohn just a reminder to review) |
devin-petersohn
approved these changes
Sep 4, 2020
Collaborator
devin-petersohn
left a comment
devin-petersohn
left a comment
There was a problem hiding this comment.
I ran some testing locally, I do not see a significant change in performance from master on default behavior (very little change if any). The refactor looks good.
LGTM, thanks @dchigarev!
aregm
pushed a commit
to aregm/modin
that referenced
this pull request
Sep 16, 2020
…roject#1894) Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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: Dmitry Chigarev dmitry.chigarev@intel.com
What do these changes do?
flake8 modinblack --check modingit commit -sskiprowsworks incorrect forread_csvin case of multiline rows #1924