Skip to content
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

align read_excel() signature with pandas 0.23.4 #415

Merged
merged 8 commits into from Jan 17, 2019

Conversation

Projects
None yet
4 participants
@eavidan
Copy link
Collaborator

commented Jan 16, 2019

What do these changes do?

Aligning read_excel signature with pandas 0.23.4

Related issue number

#414

  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • passes black --check modin/
@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-Performance-PRB/268/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-PRB/329/
Test PASSed.

no need to merge kwds and kwargs into kwargs
this also solves python 2 support build failure
@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-Performance-PRB/269/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-PRB/330/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-Performance-PRB/270/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 16, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-PRB/331/
Test PASSed.

@devin-petersohn
Copy link
Member

left a comment

Thanks for this! Left a comment for correctness.

"convert_float": convert_float,
"converters": converters,
"dtype": dtype,
"true_values": true_values,
"false_values": false_values,
"engine": engine,
"squeeze": squeeze,
"skipfooter": skipfooter,
"kwds": kwds,

This comment has been minimized.

Copy link
@devin-petersohn

devin-petersohn Jan 16, 2019

Member

I think we need to do an update instead of setting kwds=kwds here. It will treat kwds as a keyword if we do it this way.

kwargs = {...}
kwargs.update(kwds)
return cls.from_pandas(...)

This comment has been minimized.

Copy link
@eavidan

eavidan Jan 17, 2019

Author Collaborator

You are correct, this makes much more sense. I also added an update statement one level up at pandas/io since we pack everything into kwargs as well . This would prevent the same issue of kwds having a kwds keyword while sticking to the original signatures

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-Performance-PRB/277/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-PRB/338/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-Performance-PRB/278/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-PRB/339/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-Performance-PRB/279/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Merged build finished. Test PASSed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented Jan 17, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Modin-PRB/340/
Test PASSed.

@devin-petersohn
Copy link
Member

left a comment

Thanks @eavidan!

@devin-petersohn devin-petersohn merged commit 3797edf into modin-project:master Jan 17, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.