Skip to content

Add test kwarg to methods #330

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

Conversation

scottgigante-immunai
Copy link
Collaborator

@scottgigante-immunai scottgigante-immunai commented Mar 16, 2022

Prerequisite for #313

Submission type

  • This submission adds a new feature not listed above

Testing

Submission guidelines

  • This submission follows the guidelines in our Contributing document
  • I have checked to ensure there aren't other open Pull Requests for the same update/change

PR review checklist

This PR will be evaluated on the basis of the following checks:

  • The latest version of master is merged and tested

Copy link
Collaborator

@LuckyMD LuckyMD left a comment

Choose a reason for hiding this comment

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

some questions about changes in explicit parameter passing in function headers.

@LuckyMD
Copy link
Collaborator

LuckyMD commented Mar 16, 2022

@danielStrobl if this gets merged, you will need to adapt this convention into the batch removal PR as well. Just a heads up for now.

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2022

Codecov Report

Merging #330 (45b6767) into main (eb7ee58) will decrease coverage by 1.18%.
The diff coverage is 65.82%.

@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
- Coverage   86.36%   85.18%   -1.19%     
==========================================
  Files          77       77              
  Lines        1702     1735      +33     
  Branches      100      107       +7     
==========================================
+ Hits         1470     1478       +8     
- Misses        201      220      +19     
- Partials       31       37       +6     
Flag Coverage Δ
unittests 85.18% <65.82%> (-1.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
openproblems/tasks/denoising/methods/alra.py 0.00% <0.00%> (ø)
.../tasks/multimodal_data_integration/methods/scot.py 0.00% <0.00%> (ø)
.../multimodal_data_integration/methods/procrustes.py 89.47% <50.00%> (-10.53%) ⬇️
...dal_data_integration/methods/harmonic_alignment.py 78.57% <64.28%> (-9.31%) ⬇️
...tasks/regulatory_effect_prediction/methods/beta.py 84.40% <66.66%> (-1.45%) ⬇️
openproblems/tasks/label_projection/methods/mlp.py 85.71% <72.72%> (-14.29%) ⬇️
...ms/tasks/dimensionality_reduction/methods/phate.py 90.00% <77.77%> (-10.00%) ⬇️
openproblems/tasks/denoising/methods/magic.py 100.00% <100.00%> (ø)
...enproblems/tasks/denoising/methods/no_denoising.py 100.00% <100.00%> (ø)
...lems/tasks/dimensionality_reduction/methods/pca.py 100.00% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb7ee58...45b6767. Read the comment docs.

def beta(adata, n_top_genes=500, threshold=1):
adata = _atac_genes_score(adata, top_genes=n_top_genes, threshold=threshold)
def beta(adata, test=False, top_genes=500, threshold=1):
adata = _atac_genes_score(adata, top_genes=top_genes, threshold=threshold)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't know anything about this method, but would top_genes not also be a test parameter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems reasonable to me.

Copy link
Collaborator

@LuckyMD LuckyMD left a comment

Choose a reason for hiding this comment

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

Didn't know about the n_svd = n_svd or 20 notation. Pretty cool! Only 1 question about the beta function parameter top_genes.

@scottgigante-immunai scottgigante-immunai merged commit 67ce361 into openproblems-bio:main Mar 25, 2022
@scottgigante-immunai scottgigante-immunai deleted the scottgigante/feature/method_test_kwarg branch March 25, 2022 16:32
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.

4 participants