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

Add Type Annotations to pl_bolts/utils/* #455

Merged
merged 5 commits into from
Dec 20, 2020
Merged

Add Type Annotations to pl_bolts/utils/* #455

merged 5 commits into from
Dec 20, 2020

Conversation

hassiahk
Copy link
Contributor

@hassiahk hassiahk commented Dec 16, 2020

What does this PR do?

Part of #434. This PR adds type annotations to pl_bolts.utils.*

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #455 (4510e1f) into master (a6a395f) will decrease coverage by 55.84%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #455       +/-   ##
===========================================
- Coverage   80.91%   25.07%   -55.85%     
===========================================
  Files         101      101               
  Lines        5676     5671        -5     
===========================================
- Hits         4593     1422     -3171     
- Misses       1083     4249     +3166     
Flag Coverage Δ
cpu 25.07% <66.66%> (+0.12%) ⬆️
pytest 25.07% <66.66%> (+0.12%) ⬆️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pl_bolts/utils/arguments.py 20.77% <41.66%> (-75.38%) ⬇️
pl_bolts/utils/semi_supervised.py 21.87% <50.00%> (-74.90%) ⬇️
pl_bolts/utils/__init__.py 100.00% <100.00%> (ø)
pl_bolts/utils/pretrained_weights.py 100.00% <100.00%> (ø)
pl_bolts/utils/self_supervised.py 100.00% <100.00%> (ø)
pl_bolts/utils/shaping.py 36.36% <100.00%> (+6.36%) ⬆️
pl_bolts/utils/warnings.py 100.00% <100.00%> (ø)
pl_bolts/models/rl/common/agents.py 0.00% <0.00%> (-100.00%) ⬇️
pl_bolts/models/rl/dueling_dqn_model.py 0.00% <0.00%> (-100.00%) ⬇️
...l_bolts/models/rl/vanilla_policy_gradient_model.py 0.00% <0.00%> (-96.37%) ⬇️
... and 63 more

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 a6a395f...4510e1f. Read the comment docs.

@hassiahk
Copy link
Contributor Author

@akihironitta, need help with the below two errors. I am not sure how to fix them.

  • Cannot access "__init__" directly
  • Module has no attribute "_empty"

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@hassiahk Thank you for your contribution! I left some comments on the changes. Could you have a look? As for your comment below, I'll look into it later...

@akihironitta, need help with the below two errors. I am not sure how to fix them.

  • Cannot access "__init__" directly
  • Module has no attribute "_empty"

pl_bolts/utils/semi_supervised.py Outdated Show resolved Hide resolved
pl_bolts/utils/semi_supervised.py Outdated Show resolved Hide resolved
pl_bolts/utils/arguments.py Outdated Show resolved Hide resolved
pl_bolts/utils/arguments.py Show resolved Hide resolved
pl_bolts/utils/arguments.py Outdated Show resolved Hide resolved
pl_bolts/utils/semi_supervised.py Outdated Show resolved Hide resolved
pl_bolts/utils/warnings.py Outdated Show resolved Hide resolved
@akihironitta akihironitta mentioned this pull request Dec 16, 2020
17 tasks
@hassiahk
Copy link
Contributor Author

@akihironitta, I took care of your requested changes but I also had to change Y -> labels in balance_classes while annotating it as Sequence[int] or List[int] because mypy is throwing below errors:

  • Argument 1 to "len" has incompatible type "int"; expected "Sized" -> mypy is basically saying that you did not take care of Y being None.
  • Value of type "int" is not indexable -> Not sure of this.

Another workaround would be to annotate Y as plain Sequence or List. Let me know your thoughts.

@Borda Borda requested a review from akihironitta December 16, 2020 14:47
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@hassiahk Thank you very much for your work!

For Module has no attribute "_empty", let's replace inspect._empty with inspect.Parameter.empty because both should be equivalent which is defined in inspect. The same issue was also discussed in allenai/allennlp#1191 (comment).

For Cannot access "__init__" directly, I couldn't find any existing solution, so shall we just skip the line with # type: ignore...?

pl_bolts/utils/semi_supervised.py Outdated Show resolved Hide resolved
pl_bolts/utils/semi_supervised.py Outdated Show resolved Hide resolved
@hassiahk
Copy link
Contributor Author

@akihironitta, made requested changes. Kindly review it once again. Annotated X as Union[Tensor, np.ndarray] and Y as Union[Tensor, np.ndarray, Sequence[int]].

Comment on lines +36 to +38
def balance_classes(
X: Union[Tensor, np.ndarray],
Y: Union[Tensor, np.ndarray, Sequence[int]],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still concerned about if the annotated types include all the possibility, but I think these are fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will use this balance_classes and see if we can use other types as well.

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@hassiahk Thank you for your work! LGTM :]

Comment on lines 60 to 61
final_batches_x: List[Any] = [[] for i in range(nb_batches)]
final_batches_y: List[Any] = [[] for i in range(nb_batches)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these variables are the output of the function, I think they should match with the return type of the function balance_classes():

) -> Tuple[np.ndarray, np.ndarray]:

but in order to do that, you might need to change/define other variables, too. We can tighten these types later in another PR, but it'd be nice to do it in this PR. Do you think you can improve typing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will see what I can do. :)

pl_bolts/utils/semi_supervised.py Outdated Show resolved Hide resolved
@Borda Borda merged commit 2e81b75 into Lightning-Universe:master Dec 20, 2020
@hassiahk hassiahk deleted the codefix/add-type-hinting-utils branch December 22, 2020 13:59
@akihironitta akihironitta mentioned this pull request Dec 22, 2020
8 tasks
@Borda Borda added this to the v0.3 milestone Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants