-
Notifications
You must be signed in to change notification settings - Fork 0
Fix up codes type confusion #20
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
|
|
||
| levels: list[pd.Index[Any]] = list(ini.levels) | ||
| codes: list[list[int] | npt.NDArray[np.integer[Any]]] = list(ini.codes) | ||
| codes: list[npt.NDArray[np.integer[Any]]] = list(ini.codes) |
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.
I'm not sure why we had list[int] too, but removing it is correct I think (it gets cast to an array by pandas, so no need to support the list path too as that is slow because you're casting back and forth for now reason)
| def create_level_from_collection( | ||
| level: str, value: Collection[Any] | ||
| ) -> tuple[pandas.Index[Any], list[int]]: | ||
| ) -> tuple[pandas.Index[Any], npt.NDArray[np.integer[Any]]]: |
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 second return value is the codes, so should have the same type as we expect codes to have
| if not new_level.has_duplicates: | ||
| # Fast route, can just return new level and codes from level we mapped from | ||
| return new_level, list(np.arange(len(value))) | ||
| return new_level, np.arange(len(value)) |
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.
Once the type hint was correct, we could drop the cast to a list
| else: | ||
| new_level = pd.Index([value], name=level) | ||
| new_codes = list(np.zeros(ini.shape[0])) | ||
| new_codes = np.zeros(ini.shape[0], dtype=int) |
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.
Just dropping the cast to a list here as it isn't needed (the specification of the dtype wasn't something I expected, but it makes sense as the default is float, which isn't 100% correct)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## MultiIndex-set-levels #20 +/- ##
======================================================
Coverage 96.98% 96.98%
======================================================
Files 27 27
Lines 1358 1358
Branches 153 153
======================================================
Hits 1317 1317
Misses 23 23
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@crdanielbusch this fixes up the typing issues for the codes. In short, we should be able to type codes as
list[npt.NDArray[np.integer[Any]]]everywhere. I'll add some specific comments to explain more details. Once you're happy with this and have understood it, let's merge this and then we can merge #18