- Notifications
You must be signed in to change notification settings - Fork822
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Adding type hints for cleanlab/filter#598
base:master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@## master #598 +/- ##======================================= Coverage 96.23% 96.23% ======================================= Files 60 60 Lines 4705 4707 +2 Branches 817 817 =======================================+ Hits 4528 4530 +2 Misses 91 91 Partials 86 86
|
Adding the types for variables within the functions. type cheatsheet:1. np array of float type: npt.NDArray["np.floating[T]"]2. np array of int type: npt.NDArray[np.int_]3. np array of bool type: npt.NDArray[np.bool_]4. np.array of either bool or int: npt.NDArray[Union[np.bool_, np.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.
@elisno Can you approve the np arrays as I have been using them? (cheatsheet in the commit message)
cleanlab/filter.py Outdated
| @@ -298,16 +298,16 @@ class 0, 1, ..., K-1. | |||
| # Create `prune_count_matrix` with the number of examples to remove in each class and | |||
| # leave at least min_examples_per_class examples per class. | |||
| # `prune_count_matrix` is transposed relative to the confident_joint. | |||
| prune_count_matrix = _keep_at_least_n_per_class( | |||
| prune_count_matrix:npt.NDArray[np.int_] = _keep_at_least_n_per_class( | |||
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 prune_count_matrix changes the data type (from an array of int to float). Should I declare it as a float? Although the function here returns int explicitly.
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'll have to take a better look later today.
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.
@elisno did you have a chance to look at this?
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 don't see where the datatype should change in this function. I think this annotation is fine for now.
cleanlab/filter.py Outdated
| confident_joint: Optional[np.ndarray] = None, | ||
| n_jobs: Optional[int] = None, | ||
| verbose: bool = False, | ||
| ) -> np.ndarray: | ||
| ) -> npt.NDArray[Union[np.bool_, np.int_]]: | ||
| #TODO: add docstring in the same format as other functions with args and returns |
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.
Can I add to-dos wherever the doc doesn't have the same format as the other functions?
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.
Are you referring to the "shape"-format?
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 format of doc itself.
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.
Ohh right, now I see what you mean! Thank you for pointing this out!
That's not related to this PR, so you shouldn't add this particular to-do in this PR.
Feel free toopen an issue!
If you find other instances, you can add them to the same issue.
We want to address these kinds of docstrings in a separate PR.
I've updated some of the type-hints to pass more of the strict checks (not 100% just yet). CI is now failing on "Test without optional dependencies and with minimum compatible versions of dependencies", where numpy.typing is not recognized. |
No description provided.