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

Factor out inference routines #191

Merged
merged 25 commits into from
Nov 24, 2023
Merged

Conversation

adamgayoso
Copy link
Contributor

@adamgayoso adamgayoso commented Oct 20, 2023

Fixes #187

@adamgayoso
Copy link
Contributor Author

adamgayoso commented Oct 20, 2023

As an aside, it would be helpful to add https://pre-commit.ci/ as a bot so that PRs can be autofixed (and more generally, following the scverse cookiecutter https://github.com/scverse/cookiecutter-scverse)

@adamgayoso
Copy link
Contributor Author

Just want to again mention the pre-commit here https://github.com/scverse/cookiecutter-scverse/blob/main/%7B%7Bcookiecutter.project_name%7D%7D/.pre-commit-config.yaml

Ruff is really fast and adoption is rising. It can also fix a lot of the things I'm fixing myself right now :)

@adamgayoso
Copy link
Contributor Author

@BorisMuzellec I'm not sure how strongly you feel about the test_docstrings file now that ruff should be handling everything. It's failing due to no extended summary

@BorisMuzellec
Copy link
Collaborator

@BorisMuzellec I'm not sure how strongly you feel about the test_docstrings file now that ruff should be handling everything. It's failing due to no extended summary

test_docstrings mostly runs numpydoc.validate. Given that ruff runs pydocstyle I think there's no need to double-check docstrings indeed

@adamgayoso
Copy link
Contributor Author

@BorisMuzellec it will probably also help the project to use readthedocs pull request builds https://docs.readthedocs.io/en/stable/guides/pull-requests.html

And then building docs can be previewed per PR and removed from the tests. We had done this in scvi

Copy link

@umarteauowkin umarteauowkin left a comment

Choose a reason for hiding this comment

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

Great PR, really brings readability, and will be super useful to devise custom inference routines :) Thanks a lot ! Two very very minor linting suggestions, but feel free to ignore

ndarray
Estimated mean.
"""
pass

Choose a reason for hiding this comment

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

Ridiculous comment: not sure pass is needed (since there is a docstring)

pydeseq2/dds.py Outdated
)
for i in self.non_zero_idx
)
MLE_lfcs_, mu_, hat_diagonals_, converged_ = self.inference.irls(

Choose a reason for hiding this comment

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

MLE -> mle for case convention ?

Copy link
Collaborator

@BorisMuzellec BorisMuzellec left a comment

Choose a reason for hiding this comment

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

Thanks a lot @adamgayoso for this PR, and sorry for the long wait! I'm finally merging this.

@BorisMuzellec BorisMuzellec merged commit e64c413 into owkin:main Nov 24, 2023
7 checks passed
@adamgayoso adamgayoso deleted the inference branch November 28, 2023 15:36
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.

Factoring out the core implementation with a DESeqImplementation class
3 participants