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

Add adapt = False (prior_scale=1) option to lfc_shrink() #267

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

awalsh17
Copy link
Contributor

@awalsh17 awalsh17 commented Apr 2, 2024

Reference Issue or PRs

This is in reference to #264

What does your PR implement? Be specific.

This adds an option to lfc_shrink() to allow one to keep prior_scale = 1.
This is equivalent to the R package implementation of DESeq2::lfcShrink( apeAdapt = FALSE )

I tested some and the behavior is similar to the R package. But not 100%. I am not sure, but likely a difference in the parameters I passed to each.
These plots are from using the same data, contrast, and adapt = False argument.

image

@BorisMuzellec
Copy link
Collaborator

Thanks a lot for this PR @awalsh17! I'll try to add a test to automatically compare with deseq2 by the end of the week.

@BorisMuzellec
Copy link
Collaborator

I tested some and the behavior is similar to the R package. But not 100%. I am not sure, but likely a difference in the parameters I passed to each. These plots are from using the same data, contrast, and adapt = False argument.

@awalsh17 in those plots did you run lfc shrinkage starting from the same size factors, dispersions and LFCs, or did you run a DESeq2 pipeline and a PyDESeq2 pipeline independently (starting from the same data)?

@awalsh17
Copy link
Contributor Author

awalsh17 commented Apr 4, 2024

I tested some and the behavior is similar to the R package. But not 100%. I am not sure, but likely a difference in the parameters I passed to each. These plots are from using the same data, contrast, and adapt = False argument.

@awalsh17 in those plots did you run lfc shrinkage starting from the same size factors, dispersions and LFCs, or did you run a DESeq2 pipeline and a PyDESeq2 pipeline independently (starting from the same data)?

@BorisMuzellec Good point. I ran them completely independently (starting from the same data). I could repeat using the same data input to just the lfc shrinkage?

@BorisMuzellec
Copy link
Collaborator

@BorisMuzellec Good point. I ran them completely independently (starting from the same data). I could repeat using the same data input to just the lfc shrinkage?

If you have time for it, a good sanity check would be to test on real data indeed, but for this we would need to factor out differences between DESeq2 and PyDESeq2 in terms of dispersions by manually setting the same dispersions in both packages.

Otherwise, I think that the PR passing tests on the synthetic test data is good enough

@BorisMuzellec BorisMuzellec merged commit 92f1d09 into owkin:main Apr 12, 2024
6 checks passed
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