-
Notifications
You must be signed in to change notification settings - Fork 653
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
FEAT-#4144: Implement dataframe exchange protocol for pandas storage format #4150
FEAT-#4144: Implement dataframe exchange protocol for pandas storage format #4150
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4150 +/- ##
==========================================
+ Coverage 86.83% 89.29% +2.46%
==========================================
Files 207 213 +6
Lines 17033 17760 +727
==========================================
+ Hits 14790 15859 +1069
+ Misses 2243 1901 -342
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
0c4ec6e
to
f9466ab
Compare
This pull request introduces 3 alerts when merging bd1eabb5de44134b20cbc43eb8ecbee3ccc7dc46 into 62d73b7 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 025e8ede44e53350542b5d28ebb9c488ea8b8d9a into b702759 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging bc420989f538e801483983b37689d2b088d5f227 into b702759 - view on LGTM.com new alerts:
|
bc42098
to
f859a12
Compare
This pull request introduces 1 alert when merging f859a12ad77aa7584a99da4c6ae729913cc5c6ed into dd9beee - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging c9e53becbfebdb991a3d4b10c369beb7f71336d8 into 47b8a1c - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 5f17b699026ea7069257971e6a0ff4817c3ece7a into 47b8a1c - view on LGTM.com new alerts:
|
Let's keep the spec in a single file, vendored as-is from the repo. We should make the changes there and mirror them here, adhering to |
The spec is being introduced as a single file as part of #4246. |
5f17b69
to
d1df2d4
Compare
This pull request introduces 3 alerts when merging d1df2d4bf7179467017abdcfab76033af7e93793 into fc539c3 - view on LGTM.com new alerts:
|
Signed-off-by: Igoshev, Yaroslav <[email protected]>
This pull request introduces 1 alert when merging 60477dd into acd0ff9 - view on LGTM.com new alerts:
|
Signed-off-by: Igoshev, Yaroslav <[email protected]>
Signed-off-by: Igoshev, Yaroslav <[email protected]>
Signed-off-by: Igoshev, Yaroslav <[email protected]>
Signed-off-by: Igoshev, Yaroslav <[email protected]>
Signed-off-by: Igoshev, Yaroslav <[email protected]>
c29a6f5
to
44dfef8
Compare
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/exception.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/exception.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/dataframe.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/dataframe.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Igoshev, Yaroslav <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks fine, as well as the base protocol tests are passing.
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Igoshev, Yaroslav <[email protected]>
0d533a0
to
b3beb79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides #4150 (comment) - I still feel we can shorten the implementation and gain a little bit of better readability, let's discuss in that comment I linked.
Signed-off-by: Igoshev, Yaroslav <[email protected]>
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Signed-off-by: Igoshev, Yaroslav [email protected]
What do these changes do?
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/development/architecture.rst
is up-to-date