Skip to content
This repository has been archived by the owner on Oct 9, 2024. It is now read-only.

DE/DA method refactor #98

Merged
merged 9 commits into from
Apr 2, 2024
Merged

DE/DA method refactor #98

merged 9 commits into from
Apr 2, 2024

Conversation

justjhong
Copy link
Collaborator

@justjhong justjhong commented Mar 26, 2024

This PR prepares the DE and DA functions for public use:

  • Rename perform_multivariate_analysis to differential_expression
  • Change donor to sample in many places
  • Add differential_abundance method that wraps the get_aggregated_posterior method and uses a similar API to differential_expression
  • Rewrites test_models.py to use pytest parameterize, and adds tests for DA.

Addresses #94 #95

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 94.84536% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 92.24%. Comparing base (ae1f006) to head (91abaa3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
- Coverage   93.21%   92.24%   -0.97%     
==========================================
  Files           7        7              
  Lines         869      929      +60     
==========================================
+ Hits          810      857      +47     
- Misses         59       72      +13     
Files Coverage Δ
src/mrvi/_model.py 90.45% <94.84%> (-1.20%) ⬇️

... and 1 file with indirect coverage changes

src/mrvi/_model.py Outdated Show resolved Hide resolved
src/mrvi/_model.py Outdated Show resolved Hide resolved
src/mrvi/_model.py Outdated Show resolved Hide resolved
src/mrvi/_model.py Outdated Show resolved Hide resolved
src/mrvi/_model.py Show resolved Hide resolved
src/mrvi/_model.py Show resolved Hide resolved
src/mrvi/_model.py Outdated Show resolved Hide resolved
src/mrvi/_model.py Outdated Show resolved Hide resolved
Comment on lines +35 to +99
@pytest.mark.parametrize(
"setup_kwargs, de_kwargs",
[
(
{"sample_key": "sample", "batch_key": "batch"},
[
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
"add_batch_specific_offsets": True,
},
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
"lambd": 1e-1,
"add_batch_specific_offsets": True,
},
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
"filter_inadmissible_samples": True,
"add_batch_specific_offsets": True,
},
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
"add_batch_specific_offsets": False,
},
],
),
(
{
"sample_key": "sample",
"batch_key": "batch",
"continuous_covariate_keys": ["cont_cov"],
},
[
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
"add_batch_specific_offsets": False,
}
],
),
(
{"sample_key": "sample", "batch_key": "dummy_batch"},
[
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
},
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
"lambd": 1e-1,
},
{
"sample_cov_keys": ["meta1_cat", "meta2", "cont_cov"],
"store_lfc": True,
"filter_inadmissible_samples": True,
},
],
),
],
)
Copy link
Member

Choose a reason for hiding this comment

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

This is lowkey kind of hard to parse, is there a way to simplify it?

Copy link
Member

Choose a reason for hiding this comment

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

ok nvm I see what it's doing now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea..it's because its replicating the old behavior, which was not neatly combinatorial way, but I did not want to significantly change what was being tested. We can refactor it again if needed and it will be easier this time with parametrize

src/mrvi/_model.py Outdated Show resolved Hide resolved
@martinkim0 martinkim0 self-requested a review March 27, 2024 03:12
Copy link
Collaborator

@PierreBoyeau PierreBoyeau left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have some comments on differential_abudance, where I feel we could also return log enrichment scores and be more careful regarding continuous covarariates.

src/mrvi/_model.py Outdated Show resolved Hide resolved
src/mrvi/_model.py Outdated Show resolved Hide resolved
src/mrvi/_model.py Show resolved Hide resolved
src/mrvi/_model.py Show resolved Hide resolved
@justjhong justjhong requested a review from PierreBoyeau March 28, 2024 02:55
@justjhong justjhong merged commit ee602c6 into main Apr 2, 2024
7 checks passed
@justjhong justjhong deleted the jhong/methodrefactor branch April 2, 2024 18:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants