search

staircase-dev / staircase

A powerful data analysis package based on mathematical step functions. Strongly aligned with pandas.
https://staircase.dev
MIT License
59 stars 12 forks source link

Type hints WIP #154

Closed PabloRuizCuevas closed 1 year ago

PabloRuizCuevas commented 1 year ago
PabloRuizCuevas commented 1 year ago

We can let it as a draft till we complete it, or deploy the type hints progressively, in any case I will run the pre-commit in the next days.

venaturum commented 1 year ago

Thanks Pablo, I'm trying to get mypy up and running so that we can take full advantage of the type hints, but due to impending work commitments that could take a few days. At the moment running mypy on the master branch throws up a bunch of errors such as staircase\core\ops\relational.py:135: error: "Type[Series[Any]]" has no attribute "lt" - pandas Series of course does have a lt attribute, but it might have to do with the way these relational, logical and arithmetic methods are added to the Series class, so I'll have to figure out how to tell mypy to ignore certain errors.

PabloRuizCuevas commented 1 year ago

Yes, I would say to allow some mypy errors in the beginning and progressively clean them. One of the reason I raise the issue about the architecture is exactly that, it will provide lots of "undefined attribute" wanings in mypy, pycharm, intellisense etc.

PabloRuizCuevas commented 1 year ago

In any case I will do most of the typehints, but some of them I'm not 100% sure which type is, so you would need to review them. In any case mypy strict will make easy to find the hints left.

PabloRuizCuevas commented 1 year ago

Added mypy to pyproject.toml, it has multiple exceptions and there is still multiple errors, but little by little, I think we need to create a couple of custom types like:

ScFloat = union[Inf,NegInf,float]

feel free to check what does make the most sense.

venaturum commented 1 year ago

Added mypy to pyproject.toml, it has multiple exceptions and there is still multiple errors, but little by little, I think we need to create a couple of custom types like:

ScFloat = union[Inf,NegInf,float]

feel free to check what does make the most sense.

Cool, there is a file here where we can define custom types

https://github.com/staircase-dev/staircase/blob/master/staircase/_typing.py

as a direct analog for the pandas equivalent

https://github.com/pandas-dev/pandas/blob/main/pandas/_typing.py

venaturum commented 1 year ago

Hi @PabloRuizCuevas, looks like typing.Literal was introduced in Python 3.8 which is why the pipeline is failing on 3.7. I'm guessing there is a workaround but the plan is for Staircase to support Python 3.7 until the official end of life in June 2023.

I'll also try and get you the permissions to run the pipelines without requiring approval for me.

PabloRuizCuevas commented 1 year ago

Hi, yes I know that's why I put the "import from future" but not sure if that works with pytest or there is another way of making it work...

venaturum commented 1 year ago

Hi, yes I know that's why I put the "import from future" but not sure if that works with pytest or there is another way of making it work...

Looks like this package will work until support for 3.7 is dropped

https://pypi.org/project/typing-extensions/

https://stackoverflow.com/a/67193166/11472761

venaturum commented 1 year ago

@PabloRuizCuevas You may need to let the lock file overwrite your own to get past the merge commit, then redo a new one with a poetry update with the mypy additions you've made to the pyproject file.

PabloRuizCuevas commented 1 year ago

I added Literal from typing_extensions , let us see if we can merge this part already.

codecov-commenter commented 1 year ago

Codecov Report

Base: 96.04% // Head: 96.06% // Increases project coverage by +0.02% :tada:

Coverage data is based on head (98daa2f) compared to base (351b521). Patch coverage: 98.78% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #154 +/- ## ========================================== + Coverage 96.04% 96.06% +0.02% ========================================== Files 33 33 Lines 1995 2010 +15 ========================================== + Hits 1916 1931 +15 Misses 79 79 ``` | [Impacted Files](https://codecov.io/gh/staircase-dev/staircase/pull/154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [staircase/core/slicing.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL2NvcmUvc2xpY2luZy5weQ==) | `93.25% <93.33%> (+0.23%)` | :arrow_up: | | [staircase/constants.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL2NvbnN0YW50cy5weQ==) | `82.75% <100.00%> (+0.61%)` | :arrow_up: | | [staircase/core/arrays/accessor.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL2NvcmUvYXJyYXlzL2FjY2Vzc29yLnB5) | `100.00% <100.00%> (ø)` | | | [staircase/core/arrays/extension.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL2NvcmUvYXJyYXlzL2V4dGVuc2lvbi5weQ==) | `96.13% <100.00%> (+0.04%)` | :arrow_up: | | [staircase/core/layering.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL2NvcmUvbGF5ZXJpbmcucHk=) | `98.09% <100.00%> (+0.03%)` | :arrow_up: | | [staircase/core/sampling.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL2NvcmUvc2FtcGxpbmcucHk=) | `100.00% <100.00%> (ø)` | | | [staircase/core/stairs.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL2NvcmUvc3RhaXJzLnB5) | `95.90% <100.00%> (+0.05%)` | :arrow_up: | | [staircase/test\_data.py](https://codecov.io/gh/staircase-dev/staircase/pull/154/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3RhaXJjYXNlL3Rlc3RfZGF0YS5weQ==) | `95.65% <100.00%> (+0.19%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

PabloRuizCuevas commented 1 year ago

I would say is good to go, the issue failing is not a issue of this PR, but the slice function overlapping a built in function.

venaturum commented 1 year ago

Awesome, thanks Pablo, I'll tell Codacy to ignore the slice conflict.

venaturum commented 1 year ago

Hey Pablo, I noticed you were developing on your master branch. Pandas suggest an alternative workflow which is nice, where you develop on a new "feature branch", push that to a remote branch on your fork, then open a pull request from that branch. It then allows you to develop separate features at once (each on a different branch), eg i) black upgrade in precommit yaml ii) mypy added to pyproject.toml iii) type hints added. You can continue to sync your fork with the original, pull down changes to your master branch, and then merge those changes to whatever work-in-progress you have on your "feature branches". You may already be aware of this but thought I'd mention it just in case. Thanks again for this contribution, it will pave the way for more type hints in the future!

PabloRuizCuevas commented 1 year ago

Yes, I agree, no objection from my side, normally I develop in feature branches, but I wasn't thinking at the time on having different contributions at the same time. And you're welcome :) thanks for having developed this package!