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

MAINT reduce the number of arguments in _fit_rough_dispersions #166

Merged
merged 4 commits into from
Sep 25, 2023

Conversation

BorisMuzellec
Copy link
Collaborator

@BorisMuzellec BorisMuzellec commented Aug 14, 2023

What does your PR implement? Be specific.

This PR is a minor refactoring of DeseqDataSet._fit_rough_dispersions().

Currently, this method takes both counts and size_factors as arguments, only to compute normalized factors as counts / size_factors[:, None].

We now provide normed_counts directly, which are already stored in self.layers["normed_counts"] when _fit_rough_dispersions() is called in the pydeseq2 pipeline.

Likewise, we provide normed_counts instead of counts in _fit_moments_dispersions() to avoid performing the division again.

…y replacing ``counts`` and ``size_factors`` with ``normed counts``
@BorisMuzellec BorisMuzellec requested a review from a user August 14, 2023 09:58
@BorisMuzellec BorisMuzellec changed the title Maint reduce the number of arguments in _fit_rough_dispersions MAINT reduce the number of arguments in _fit_rough_dispersions Aug 14, 2023
Copy link
Collaborator

@maikia maikia left a comment

Choose a reason for hiding this comment

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

thanks @BorisMuzellec overall it LGTM

@BorisMuzellec BorisMuzellec merged commit 904357e into main Sep 25, 2023
8 checks passed
@BorisMuzellec BorisMuzellec deleted the refactor_fit_rough_dispersions branch September 25, 2023 08:05
@BorisMuzellec
Copy link
Collaborator Author

Thanks @maikia!

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