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 hints #27

Merged
merged 4 commits into from
Nov 10, 2022
Merged

add type hints #27

merged 4 commits into from
Nov 10, 2022

Conversation

plming
Copy link
Contributor

@plming plming commented Nov 9, 2022

I've added type hints for parameters and returns.
I referenced mostly docstring, implementation at few points.
It doesn't affect any runtime behavior and I checked all test passed!

@mthh
Copy link
Owner

mthh commented Nov 9, 2022

Thank you! This is something I had thought of but didn't jump into!

Your type hints seem intuitively correct but when testing with mypy I have some errors. I think it's due to inconsistencies between some of my code and the documentation in the docstrings (which must have guided your additions).
I will just play a bit with the type hints and mypy to see if I merge it as it or if I take the opportunity to modify the docstrings and some of your hint types.

@mthh
Copy link
Owner

mthh commented Nov 9, 2022

Is this OK if I push further modifications to your type-hint branch so that we can track it on the same PR ?

@plming
Copy link
Contributor Author

plming commented Nov 9, 2022

Sure! Thanks for working :)

@mthh
Copy link
Owner

mthh commented Nov 9, 2022

I changed a few things :

  • the predict method is also working with a single scalar value,
  • the group method returns a List of numpy arrays,
  • most of the functions were documented as working with "an iterable sequence of number", which was misleading because they all the sequences are iterables, and most of the function are only working with Sequence (they dont work with something like (i for i in range(100)) expect the predict one), so I changed the Iterable typing to Sequence in the appropriate places.

Are you OK with my changes ?

@plming
Copy link
Contributor Author

plming commented Nov 10, 2022

All good! But before we go, I fixed docstring with exact type hints. Please check it, and then fine to merge it!

@mthh
Copy link
Owner

mthh commented Nov 10, 2022

Indeed, thanks!

And thank you for this useful contribution!

I will release a new version later today.

@mthh mthh merged commit 1951332 into mthh:master Nov 10, 2022
@plming plming deleted the type-hint branch November 10, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants