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

Implement Type Hints #441

Merged
merged 37 commits into from Jan 10, 2023
Merged

Implement Type Hints #441

merged 37 commits into from Jan 10, 2023

Conversation

martinvonk
Copy link
Collaborator

@martinvonk martinvonk commented Dec 27, 2022

Short Description

This pull request implements Type Hints into Pastas. Python does not enforce function and variable type annotations. However they can be used by third party tools such as type checkers, IDEs, linters, etc. This helps the Pastas users and developers to write better code.

Checklist before PR can be merged:

Deprecated since version 1.5.0: iteritems is deprecated and will be removed in a future version. Use .items instead.
remove api changes since they are generated automatically
@martinvonk martinvonk linked an issue Dec 27, 2022 that may be closed by this pull request
@martinvonk martinvonk changed the base branch from master to dev December 27, 2022 09:39
@martinvonk
Copy link
Collaborator Author

I spent a lot of time in public transport this Christmas so this is the end result 🎁. I did not implement the type annotation in the read functions since they will be deprecated in the future.

@martinvonk martinvonk added this to the 1.0: Arrabiata milestone Dec 27, 2022
@martinvonk martinvonk added the enhancement Indicates improvement of existing features label Dec 27, 2022
@martinvonk martinvonk changed the title 389 typehinting Implement Type Hints Dec 28, 2022
@martinvonk
Copy link
Collaborator Author

The read the docs test fails because it uses a numpy version > 1.23 and numba does not support it yet.
From numba/numba#8615 :

For devs who are trying to fix their environments
The problem occurs in the latest version of numpy. Installing a lower version of numpy (<1.24) fixed the problem for us.

It is being worked on:
numba/numba#8620

For now I'll try and fix it in the requirements by forcing numpy < 1.24

numpy < 1.24, due to numba support
numpy < 1.24, due to numba support
@martinvonk
Copy link
Collaborator Author

This can be merged into the dev branch after a new version of Pastas is released (at the latest next Tuesday) by @raoulcollenteur.

Maybe @dbrakenhoff can check this PR?

martinvonk added a commit that referenced this pull request Dec 29, 2022
See comments on Numba Support of Numpy 1.24 in #441
@raoulcollenteur
Copy link
Member

The read the docs test fails because it uses a numpy version > 1.23 and numba does not support it yet. From numba/numba#8615 :

For devs who are trying to fix their environments
The problem occurs in the latest version of numpy. Installing a lower version of numpy (<1.24) fixed the problem for us.

It is being worked on: numba/numba#8620

For now I'll try and fix it in the requirements by forcing numpy < 1.24

Nice catch. Should we not directly change this in the dev-version, rather than in every single PR?

@martinvonk
Copy link
Collaborator Author

martinvonk commented Dec 29, 2022

Nice catch. Should we not directly change this in the dev-version, rather than in every single PR?

I don't have permission to write directly to the dev branch :), so I'd just merge the #420 PR to the dev and everything will be solved. I'll remove the change from this PR #441 afterwards.

I only fixed the same bug in #420 because it can't be merged otherwise.

pastas/model.py Outdated Show resolved Hide resolved
pastas/typing/types.py Outdated Show resolved Hide resolved
Copy link
Member

@dbrakenhoff dbrakenhoff left a comment

Choose a reason for hiding this comment

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

Looking much better already. Nice move with the quoted annotations in typing.py and the mypy trick.

I think we should replace Tminmax with a normal Timestamp (or Union[str, Timestamp]), because those are functionally the same types.

Also nitpicking, but perhaps rename Array_Like to ArrayLike (more in line with Python class naming conventions)?

I agree on the black formatting style for functions with lots of input parameters. Whether we should apply it to the whole code base is a separate discussion maybe.

pastas/typing/__init__.py Outdated Show resolved Hide resolved
pastas/stats/tests.py Outdated Show resolved Hide resolved
pastas/stats/tests.py Outdated Show resolved Hide resolved
pastas/stats/tests.py Outdated Show resolved Hide resolved
pastas/stressmodels.py Outdated Show resolved Hide resolved
pastas/stats/metrics.py Outdated Show resolved Hide resolved
pastas/plots.py Outdated Show resolved Hide resolved
pastas/modelstats.py Outdated Show resolved Hide resolved
pastas/model.py Outdated Show resolved Hide resolved
martinvonk and others added 3 commits January 9, 2023 13:52
- remove None default arguments that are not needed
- add Optional[Series] if need be
- rename Array_Like to ArrayLike
- rename Tminmax to TimestampType
@martinvonk
Copy link
Collaborator Author

How about formatting this pull request with Black? I feel like methods and functions with type hinting become much more readable with Black.

I was crazy enough to format the functions and methods by hand. Leaving the rest of the code (and thus git blame) intact which is one the arguments @dbrakenhoff raises in #448

@raoulcollenteur
Copy link
Member

Don't know how to rerun all GH action jobs without 3.11.

@raoulcollenteur raoulcollenteur removed the request for review from dbrakenhoff January 10, 2023 10:49
@raoulcollenteur raoulcollenteur merged commit 900a182 into dev Jan 10, 2023
@raoulcollenteur raoulcollenteur deleted the 389-typehinting branch January 10, 2023 10:59
@martinvonk martinvonk self-assigned this Jan 19, 2023
raoulcollenteur added a commit that referenced this pull request Jan 26, 2023
Finally after 5 years fix issue #441
@martinvonk martinvonk added code quality Related to code quality, testing, CI, project setup etc. and removed enhancement Indicates improvement of existing features labels Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Related to code quality, testing, CI, project setup etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Implement Type Hints into Pastas
3 participants