Add Index unique #912
Add Index unique #912
Conversation
|
emm, any idea how to fix |
|
@charlesdong1991 Thanks for working on this! |
|
ahh, thanks for your quick reply! @ueshin |
Codecov Report
@@ Coverage Diff @@
## master #912 +/- ##
=========================================
- Coverage 94.47% 94.37% -0.1%
=========================================
Files 34 34
Lines 6392 6405 +13
=========================================
+ Hits 6039 6045 +6
- Misses 353 360 +7
Continue to review full report at Codecov.
|
| @@ -140,6 +140,21 @@ def test_multi_index_names(self): | |||
| with self.assertRaises(PandasNotImplementedError): | |||
| kidx.name = 'renamed' | |||
|
|
|||
| def test_index_unique(self): | |||
There was a problem hiding this comment.
Sorry if I missed but did we have a test case for multi-index?
There was a problem hiding this comment.
no, i think their implementation is slightly different, and i thought the scope of this PR is for Index, not MultiIndex (I was supposed to do in a separate PR).
But do you prefer to have this unique function added for both Index and MultiIndex in this PR? @HyukjinKwon
There was a problem hiding this comment.
Ah, it's fine to target to implement it only in Index. Can we explicitly throw a notimplemented exception manually?
There was a problem hiding this comment.
i added a PandasNotImplementedError, pls let me know if this is what you meant, thanks.
|
any follow up review will be appreciated! :) |
| @@ -104,7 +104,6 @@ class _MissingPandasLikeIndex(object): | |||
| to_numpy = unsupported_function('to_numpy') | |||
| transpose = unsupported_function('transpose') | |||
| union = unsupported_function('union') | |||
| unique = unsupported_function('unique') | |||
| value_counts = unsupported_function('value_counts') | |||
| view = unsupported_function('view') | |||
| where = unsupported_function('where') | |||
There was a problem hiding this comment.
@charlesdong1991, sorry can you remove unique at _MissingPandasLikeMultiIndex as well?
Techinically MultiIndex has the method called unique. It's a bit complicated because MultiIndex inherits Index, so I suggested to throw an exception :-)..
There was a problem hiding this comment.
ok, removed! @HyukjinKwon
i will get some free time recently, and see if I could add some properties and functions for Index and MultiIndex and then implement unique for MultiIndex if itholic hasn't implemented them all beforehand xD 😝
|
LGTM. I will merge once the conflicts are resolved. |
Softagram Impact Report for pull/912 (head commit: 17c709e)⭐ Change Overview
📄 Full report
Impact Report explained. Give feedback on this report to [email protected] |
|
oops, should have merged to master before pushing commits. |
#880