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

Explicitly not support codes and levels property for MultiIndex #952

Merged
merged 8 commits into from
Nov 18, 2019

Conversation

charlesdong1991
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 25, 2019

Codecov Report

Merging #952 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #952   +/-   ##
=======================================
  Coverage   95.13%   95.13%           
=======================================
  Files          34       34           
  Lines        6765     6765           
=======================================
  Hits         6436     6436           
  Misses        329      329
Impacted Files Coverage Δ
databricks/koalas/missing/indexes.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0fc9af...d3fd4c0. Read the comment docs.

i += 1

sdf = sdf.orderBy("__order__").select(sdf.colRegex("`__code_.`"))
return DataFrame(_InternalFrame(sdf=sdf)).astype('int').to_numpy().T.tolist()
Copy link
Member

Choose a reason for hiding this comment

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

Hmmmmm... this collects everything into driver side which can be against the design principles ... I think we might have to rather explicitly don't support, and let users call to_pandas() directly .. ? cc @ueshin

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, in that case, we might also rather explicitly not support levels.

Copy link
Contributor Author

@charlesdong1991 charlesdong1991 Nov 11, 2019

Choose a reason for hiding this comment

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

emm, I see. Then if so, what do you suggest to do next? @ueshin @HyukjinKwon

In my work experience, I think most of the time, those method/property related to index are used on columns, and pandas should be enough.

Copy link
Member

Choose a reason for hiding this comment

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

It requires to collect all data into driver so we shouldn't implement. @charlesdong1991 do you use this property and levels often? If not, let's explicitly don't support. You can refer #744 to explicitly don't support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for my late reply, didn't have time these days @HyukjinKwon

tbh, i barely use this levels as well as codesoften, but this can be different across different people and work. I will also drop supports for levels as well

@charlesdong1991 charlesdong1991 changed the title Add codes property for MultiIndex Explicitly not support codes property for MultiIndex Nov 16, 2019
@charlesdong1991 charlesdong1991 changed the title Explicitly not support codes property for MultiIndex Explicitly not support codes and levels property for MultiIndex Nov 16, 2019
@softagram-bot
Copy link

Softagram Impact Report for pull/952 (head commit: d3fd4c0)

⭐ Change Overview

Showing the changed files, dependency changes and the impact - click for full size
(Open in Softagram Desktop for full details)

📄 Full report

Impact Report explained. Give feedback on this report to support@softagram.com

'levels',
reason="'levels' requires to collect all data into the driver which is against the "
"design principle of Koalas. Alternatively, you could call 'to_pandas()' and"
" use 'levels' property in pandas.")
Copy link
Member

Choose a reason for hiding this comment

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

@ueshin let me merge this one for now .. but we can always flip this decision if I missed anything.

@HyukjinKwon
Copy link
Member

Thanks @charlesdong1991

@HyukjinKwon HyukjinKwon merged commit 169e38e into databricks:master Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants