-
Notifications
You must be signed in to change notification settings - Fork 695
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
Propr propr #4817
Propr propr #4817
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work- very tidy. Just curious why you're not snapshotting or otherwise checking the other outputs?
then { | ||
assertAll( | ||
{ assert process.success }, | ||
{ assert snapshot(process.out.matrix).match("Test propr/propr using default options") }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only snapshotting the matrix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FDR file is calculated based on a random sampling, so I don´t think we can snapshot.
Then the rds is the R object including the matrix, and the fdr, and more stuff that could be useful to run propr again if the user wants. So, same problem cannot be snapshot and have the same md5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use set.seed() to make the FDR reproducible? You're right on the rds of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require to change the package code and rebuild container and so on...
It is really a test that shuffles the input data and run multiple times the function used to create the matrix output, so I think if the matrix output is consistent, this step should not have problem too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to check this file in some way I think.
One thing you could do is e.g. extract the last line of the file, and check that the first value matches a test value to a limited number of decimal places (hopefully the FDR is stable to a limited extent). You could adapt from https://nf-co.re/docs/contributing/tutorials/nf-test_assertions#file-contains-check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinin4fjords now it should be fine. I added a way to fix the seed for fdr reproducibility
tests/config/pytest_modules.yml
Outdated
@@ -1,2619 +1,3577 @@ | |||
adapterremovalfixprefix: | |||
- modules/nf-core/adapterremovalfixprefix/** | |||
- tests/modules/nf-core/adapterremovalfixprefix/** | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think you need to be changing this file- probably just something funny on your branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cuz it always give me merging conflicts with the master... what should I do then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably something funny in your git history. Try literally copying in the file from master in a new commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sorting out the seed - I think that really helps. You're just missing the FDR check on the first test.
assertAll( | ||
{ assert process.success }, | ||
{ assert snapshot(process.out.matrix).match("Test propr/propr using default options - matrix") }, | ||
{ assert snapshot(process.out.versions).match("versions") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing fdr check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey there! fdr is an optional output actually. Here in the default test, I did not ask to compute fdr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, my bad, thanks for explanation
assertAll( | ||
{ assert process.success }, | ||
{ assert snapshot(process.out.matrix).match("Test propr/propr while running clr+pcor.bshrink explicitly - matrix")}, | ||
{ assert snapshot(process.out.fdr).match("Test propr/propr while running clr+pcor.bshrink explicitly - fdr")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conventionally, we have a version check in every test. But I won't insist.
@pinin4fjords Thank you for the review! |
* modified affy/justrma to allow the user get the unlog data when --keep.log2 FALSE * after prettier * trailing whitespace * added final newline * fix issue * updated snapshot * corrected md5sum issues with inconsistent decimals * corrected round matrix method * . * copied pytest_modules.yml * . * updated container version and added reproducible test for fdr
* modified affy/justrma to allow the user get the unlog data when --keep.log2 FALSE * after prettier * trailing whitespace * added final newline * fix issue * updated snapshot * corrected md5sum issues with inconsistent decimals * corrected round matrix method * . * copied pytest_modules.yml * . * updated container version and added reproducible test for fdr
Add propr/propr module to perform compositional data analysis on rna-seq data
PR checklist
Closes #XXX
versions.yml
file.label
PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware