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.

Sign up for GitHub

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

Jump to bottom

FEAT-#7207: Add the ability to assing a df to a columns selection without d2p #7210

Merged
merged 8 commits into from
Apr 26, 2024
Merged

FEAT-#7207: Add the ability to assing a df to a columns selection without d2p #7210

merged 8 commits into from
Apr 26, 2024

Conversation

YarShev
Copy link
Collaborator

@YarShev YarShev commented Apr 22, 2024

What do these changes do?

  • first commit message and PR title follow format outlined here

    NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves BUG: Falling back to standard Pandas implementation when assigning a dataframe to a columnar selection #7207
  • tests passing
  • module layout described at docs/development/architecture.rst is up-to-date

@YarShev
FEAT-modin-project#7207: Add the ability to assing a df to a columns …
97ea4bf 
…selection without d2p

Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
@YarShev
Add a test
878c295 
Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
@YarShev
Fix HDK
ff8df2f 
Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
@YarShev
Remove workaround for categorical dtypes for HDK
e0f2fb3 
Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
@YarShev
Fix HDK
f3eac3f 
Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
@YarShev
Fix lint
41b6170 
Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
AndreyPavlenko
AndreyPavlenko previously approved these changes Apr 23, 2024
View reviewed changes
anmyachev
anmyachev reviewed Apr 25, 2024
View reviewed changes
modin/pandas/dataframe.py Outdated
@@ -2709,6 +2711,7 @@ def __setitem__(self, key, value) -> None:
to_concat.append(src_obj[to_take])

new_qc = concat(to_concat, axis=1)._query_compiler
new_qc.index = self.index
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Collaborator Author

@YarShev YarShev Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
new_qc.index = self.index

I had some failures in HDK tests without this change. Let's try to revert to see if latest changes helped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HDK failed without this change so reverting it back.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to fix the code only for the HDK, because here we will slow down all implementations due to the fact that the index is propagated to all partitions, and this is not a cheap operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been able to reproduce the failures locally, the tests passed to me. @AndreyPavlenko, do you have any thoughts why this happens? I remember you debugged some similar case when there was a failure in CI but locally the tests passed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@YarShev
Fix HDK
2c8fdb1 
Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
@YarShev
Enable tests for HDK
ba655e4 
Signed-off-by: Igoshev, Iaroslav <iaroslav.igoshev@intel.com>
anmyachev
anmyachev approved these changes Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@anmyachev anmyachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@anmyachev anmyachev merged commit 16fe85f into modin-project:main Apr 26, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmyachev anmyachev anmyachev approved these changes

@AndreyPavlenko AndreyPavlenko AndreyPavlenko left review comments

@devin-petersohn devin-petersohn Awaiting requested review from devin-petersohn devin-petersohn is a code owner

@mvashishtha mvashishtha Awaiting requested review from mvashishtha mvashishtha is a code owner

@RehanSD RehanSD Awaiting requested review from RehanSD RehanSD is a code owner

@vnlitvinov vnlitvinov Awaiting requested review from vnlitvinov vnlitvinov is a code owner

@dchigarev dchigarev Awaiting requested review from dchigarev dchigarev is a code owner

@aregm aregm Awaiting requested review from aregm

@gshimansky gshimansky Awaiting requested review from gshimansky

@ienkovich ienkovich Awaiting requested review from ienkovich

@Garra1980 Garra1980 Awaiting requested review from Garra1980

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Falling back to standard Pandas implementation when assigning a dataframe to a columnar selection
3 participants
@YarShev @AndreyPavlenko @anmyachev