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.

Sign up for GitHub

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

Jump to bottom

compute DE+ and DE- separately #2826

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

compute DE+ and DE- separately #2826

wants to merge 5 commits into from

Conversation

LevyNat
Copy link
Collaborator

@LevyNat LevyNat commented Jun 3, 2024

No description provided.

@canergen
Copy link
Member

canergen commented Jun 3, 2024

Fixed the wrong behavior that we have seen in different models (high significance despite low mean LFC but high standard deviation). It still requires a release note and it would be good to check in our DE tutorial whether things are changing there. I’m not sure that the Bayes factor computation is correct @PierreBoyeau?

@PierreBoyeau
Copy link
Contributor

Hi, LGTM. I am unsure about the Bayes factor here, since we don't have two, but three competing models now(equally expressed, upreg, and downreg). I would be in favor of not computing BFs in this case.

@canergen
Copy link
Member

canergen commented Jun 7, 2024

I guess it's fine to remove. I would assume we do two tests: upregulated vs rest and downregulated vs rest. We output only the winner of the test for upregulated/downregulated. I guess the corresponding Bayes factor for the returned FDR is then: upregulated/(equally+downregulated) or downregulated/(equally+upregulated).

@martinkim0 martinkim0 added the P0 label Jul 12, 2024
canergen
canergen reviewed Aug 3, 2024
View reviewed changes
src/scvi/model/base/_differential.py Outdated
@@ -360,7 +365,8 @@ def m1_domain_fn(samples):
res = dict(
proba_de=proba_m1,
Copy link
Member

Choose a reason for hiding this comment

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

Why not return probe_de?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@canergen canergen canergen left review comments

@PierreBoyeau PierreBoyeau Awaiting requested review from PierreBoyeau

@martinkim0 martinkim0 Awaiting requested review from martinkim0

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
P0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LevyNat @canergen @PierreBoyeau @martinkim0 @ori-kron-wis