ENH Support type hinting using `mypy` by illumination-k · Pull Request #68 · scverse/PyDESeq2

illumination-k · 2023-02-02T06:27:53Z

Reference Issue or PRs

Please see #39

What does your PR implement? Be specific.

This PR support type hinting by mypy and fixed some docstring which incompatible with mypy type checking.

BorisMuzellec · 2023-02-02T09:01:54Z

Hi @illumination-k, thanks a lot for your work!

This is our first contribution from an external collaborator, we're very excited about it :).

I have just approved the CI to run on this PR. It seems that some checks have failed, could you have a look at it if you have time?

FYI I'll be away next week so your interlocutor will probably be @maikia.

Thanks again for your help, hope to merge this soon!

illumination-k · 2023-02-02T10:11:45Z

Sorry for failing CI. I ran CI and pre-commit scripts manually and maybe OK. If failed, I will check tomorrow.

maikia · 2023-02-06T15:53:32Z

Hi @illumination-k
Thanks for your contribution! I think accidentally you added docs/auto_examples (they are generated by sphinx when make html is run and should not be a part of git). Can you remove them and I will review your PR?

illumination-k · 2023-02-09T01:23:20Z

Sorry @maikia, I missed your comments. I removed auto_examples. Should we add them in .gitignore?

maikia

@illumination-k thanks again for your contribution. Very nice and through PR.

Could you provide me with the details on your settings of mypy, or even better if you can add it to the CI (and I will reproduce the steps locally).
ATM if I run naive mypy it fails with errors, eg

To my knowledge those errors can also be coming from the version of Python. The one I am using locally (as well as the one used in our CI) is 3.8.

I also didn’t realise before but api/docstrings should also be removed from the PR, and yes, adding this as well as auto examples to .gitignore is a great idea. Feel free to open a new PR if you like.

illumination-k · 2023-02-14T11:23:51Z

I used Python 3.10, and you are right python 3.8 dose not support direct list, tuple. Now I fixed type hinting for Python 3.8

erge branch 'main' into feature/mypy

maikia

@illumination-k thanks again for your PR! I extended it to have mypy running within CI and corrected a bug coming from one of the changes in the code. Can you please make sure that you agree with all the changes I did?

@BorisMuzellec @mandreux-owkin can one of you review before merge (as I committed to this PR I prefer that there is someone else checking if all is good, thx)

BorisMuzellec

LGTM, up to minor suggestions.

Thanks a lot @illumination-k (and @maikia) for the work!

pydeseq2/dds.py

pydeseq2/utils.py

BorisMuzellec · 2023-02-16T10:27:40Z

Hi, now that the AnnData integration has been merged, there will be numerous merge conflicts to resolve. I'll handle them, if that's OK for you.

illumination-k · 2023-02-16T13:55:47Z

Sure! Sorry for my late response, @BorisMuzellec .

illumination-k · 2023-02-17T08:29:43Z

Your commit maybe inactivated type hinting. There are many Any types. I think that in order to fully benefit from type hints with mypy, you should use a command such as mypy . --check-untyped-defs --strict.

Would it be better to close this PR and create a new one, or create new commit in this PR?

BorisMuzellec · 2023-02-17T08:59:52Z

Hi @illumination-k, indeed, thanks for checking!

It seems like I made some git manipulation mistakes, sorry for that...

I don't think it's necessary to close this PR. I'll just revert to where it was before I made changes and take it from there. We'll squash anyways at merge time.

This reverts commit 1866117.

This reverts commit 5f44d01.

This reverts commit b40b88a.

maikia

This PR did grow quite a bit. LGTM. just few minor comments.
@illumination-k can you also comment if you agree with all the changes done to your PR?

docs/source/conf.py

pydeseq2/utils.py

maikia · 2023-02-17T10:20:41Z

@BorisMuzellec there are also some conflicts needing resolve

BorisMuzellec · 2023-02-17T10:32:09Z

@BorisMuzellec there are also some conflicts needing resolve

Yes, I am currently handling those. These are due to the anndata merge

illumination-k · 2023-02-17T11:00:24Z

I would prefer to address mypy . --check-untyped-def and ignore untyped things coming from anndata integration in this PR. Because there are many untyped things and it is pretty tough work. I'm not so familiar with anndata, but it seems not to support type stubs, so maybe some work is also need to handle anndata type.

We should address anndata typing and --strict flag in the next PRs.

BorisMuzellec · 2023-02-17T13:34:46Z

Hi @illumination-k and @maikia, I did my best to pass the the mypy . --check-untyped-defs (without the strict option).
Could you have a look?

I agree with @illumination-k that we should deal with more intricate typing issues brought by the anndata integration in a new PR.

maikia · 2023-02-17T14:41:03Z

@BorisMuzellec I left some comments in my previous review which were still not addressed. Otherwise I agree with your last comment

Co-authored-by: Maria Telenczuk <maja_ka@hotmail.com>

BorisMuzellec · 2023-02-20T08:40:00Z

@illumination-k and @maikia since all comments are now resolved, unless there is something else you would like to change I'll merge this PR today

illumination-k added 5 commits February 2, 2023 14:12

type hinting for dds.py

3401fab

type hinting for ds.py

ae00ca1

type hinting for grid_search.py

d9b0e71

type hinting for utils.py

34445c4

precommit

6579aca

BorisMuzellec requested a review from maikia February 2, 2023 09:02

illumination-k added 2 commits February 2, 2023 18:48

fix typo

af128e0

run precommit script manually and fix

7e428c7

update docs and conf

4cbcad7

remove auto_example

1d46f73

ghost mentioned this pull request Feb 9, 2023

[FIX] Adds docs' auto-generated rst files to the gitignore #73

Closed

maikia reviewed Feb 10, 2023

View reviewed changes

illumination-k added 2 commits February 14, 2023 19:02

pull upstream and merge confilict, removing api/docstring

7534f47

fix for python v3.8

d98e99e

maikia added 3 commits February 14, 2023 17:00

add mypy to precommit

fda48d7

merge with main

20a9c0f

removing bugs

3213af6

maikia mentioned this pull request Feb 15, 2023

CI update workflow #80

Merged

merge with main

2b82daa

erge branch 'main' into feature/mypy

maikia approved these changes Feb 15, 2023

View reviewed changes

BorisMuzellec requested changes Feb 15, 2023

View reviewed changes

pydeseq2/dds.py Outdated Show resolved Hide resolved

pydeseq2/dds.py Outdated Show resolved Hide resolved

pydeseq2/dds.py Outdated Show resolved Hide resolved

pydeseq2/utils.py Outdated Show resolved Hide resolved

Boris MUZELLEC added 2 commits February 16, 2023 15:30

resolve merge conflicts

b40b88a

resolve merge conflicts

5f44d01

resolve merge conflicts

1866117

BorisMuzellec requested a review from arthurPignetOwkin as a code owner February 16, 2023 14:32

BorisMuzellec requested review from a user and maikia February 16, 2023 14:32

BorisMuzellec approved these changes Feb 16, 2023

View reviewed changes

Boris MUZELLEC added 3 commits February 17, 2023 10:03

Revert "resolve merge conflicts"

a9ebaef

This reverts commit 1866117.

Revert "resolve merge conflicts"

7a8c230

This reverts commit 5f44d01.

Revert "resolve merge conflicts"

07ef994

This reverts commit b40b88a.

maikia approved these changes Feb 17, 2023

View reviewed changes

docs/source/conf.py Show resolved Hide resolved

pydeseq2/utils.py Outdated Show resolved Hide resolved

pydeseq2/utils.py Outdated Show resolved Hide resolved

pydeseq2/utils.py Outdated Show resolved Hide resolved

Boris MUZELLEC added 5 commits February 17, 2023 12:02

fix: resolve merge conflicts

512e26d

fix: handle mypy . --check-untyped-def errors

b37c7fd

fix: black linting

55194ba

docs: add .rst files since these are not generated automatically

a3476c2

fix: typo

d534824

BorisMuzellec requested a review from maikia February 17, 2023 13:56

fix: typo

5f278a6

Co-authored-by: Maria Telenczuk <maja_ka@hotmail.com>

BorisMuzellec changed the title ~~Support type hinting by mypy~~ ENH Support type hinting using mypy Feb 17, 2023

fix: remove unneeded # type: ignore

ad1c948

BorisMuzellec merged commit 9d56d33 into scverse:main Feb 20, 2023

BorisMuzellec mentioned this pull request Feb 20, 2023

DOC fix broken RTD example gallery #83

Merged

illumination-k mentioned this pull request Feb 21, 2023

Support disallow-untyped flags of mypy #85

Closed

BorisMuzellec mentioned this pull request Apr 26, 2023

Support type hinting #39

Closed

Conversation

illumination-k commented Feb 2, 2023

Reference Issue or PRs

What does your PR implement? Be specific.

Uh oh!

BorisMuzellec commented Feb 2, 2023

Uh oh!

illumination-k commented Feb 2, 2023

Uh oh!

maikia commented Feb 6, 2023

Uh oh!

illumination-k commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maikia left a comment

Choose a reason for hiding this comment

Uh oh!

illumination-k commented Feb 14, 2023

Uh oh!

maikia left a comment

Choose a reason for hiding this comment

Uh oh!

BorisMuzellec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BorisMuzellec commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illumination-k commented Feb 16, 2023

Uh oh!

illumination-k commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BorisMuzellec commented Feb 17, 2023

Uh oh!

maikia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maikia commented Feb 17, 2023

Uh oh!

BorisMuzellec commented Feb 17, 2023

Uh oh!

illumination-k commented Feb 17, 2023

Uh oh!

BorisMuzellec commented Feb 17, 2023

Uh oh!

maikia commented Feb 17, 2023

Uh oh!

BorisMuzellec commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

illumination-k commented Feb 9, 2023 •

edited

Loading

BorisMuzellec commented Feb 16, 2023 •

edited

Loading

illumination-k commented Feb 17, 2023 •

edited

Loading

BorisMuzellec commented Feb 20, 2023 •

edited

Loading