-
Notifications
You must be signed in to change notification settings - Fork 367
Add type annotations to Index and MultiIndex #1882
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
Conversation
| index_map[i], index_map[j], = index_map[j], index_map[i] | ||
| result = DataFrame(self._kdf._internal.copy(index_map=OrderedDict(index_map))).index | ||
| return result | ||
| return result # type: ignore |
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.
Otherwise, Mypy expects "Index" as the return type.
| index_map = OrderedDict(zip(sdf.columns, names)) | ||
| internal = InternalFrame(spark_frame=sdf, index_map=index_map) | ||
| return DataFrame(internal).index | ||
| return DataFrame(internal).index # type: ignore |
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.
Otherwise, Mypy expects "Index" as the return type.
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.
Oh, so maybe it seems that mypy requires us to add a kind of abstract methods to the Index class ?
Maybe can we ignore that rule like we did for inplace parameter ?? 😲
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.
Thanks @itholic, this is a good idea!
Unfortunately, I didn't find a parameter in https://mypy.readthedocs.io/en/stable/config_file.html can ignore this rule.
I do agree this should a common problem for other projects. Let me do more search and see if there is a better solution.
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.
Yep, Sure :)
Codecov Report
@@ Coverage Diff @@
## master #1882 +/- ##
==========================================
- Coverage 94.20% 91.31% -2.89%
==========================================
Files 40 40
Lines 9915 9833 -82
==========================================
- Hits 9340 8979 -361
- Misses 575 854 +279
Continue to review full report at Codecov.
|
itholic
left a comment
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.
Seems fine to me except one question
databricks/koalas/indexes.py
Outdated
| return isinstance(self.spark.data_type, IntegralType) | ||
|
|
||
| def item(self): | ||
| def item(self) -> Union[Scalar, Tuple]: |
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.
Actually using Scalar is strictly not correct because this can return a pandas instance too. For exmaple, pd.Timestamp which is not in the Scalar from my cursory reading. But I think it's okay - we can fix it later separately.
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.
This makes sense. How about we define a pandas Scalar separately? @HyukjinKwon
|
Looks pretty good |
4ef7df9 to
dda6744
Compare
No description provided.