-
Notifications
You must be signed in to change notification settings - Fork 8
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
[BENCHMARK] tool comparison macro benchmarks #156
[BENCHMARK] tool comparison macro benchmarks #156
Conversation
Codecov Report
@@ Coverage Diff @@
## master #156 +/- ##
=======================================
Coverage 94.93% 94.93%
=======================================
Files 18 18
Lines 731 731
=======================================
Hits 694 694
Misses 37 37 Continue to review full report at Codecov.
|
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.
Nice work! You already have a nice structure set up, having a separate configuration file and directories for rules and scripts. Might also be good to extract the rules .smk files from /parameter_benchmarks/Snakefile
.
I've added some comments, but might have misunderstood your init process :)
params: | ||
sample = config["parameters"]["sample"], | ||
min_qual = config["parameters"]["min_qual"], | ||
min_var_length = config["parameters"]["min_var_length"], |
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.
Since the same parameter is used for multiple rules you could define e.g min_var_length
globally (before the rules) for the whole .smk file and use it in the shell call as {min_var_length}
instead of {params.min_var_length}
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.
Hmm, I'm not sure, if this makes it more clear. What do you say @joergi-w ?
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.
I agree with @eaasna that all parameters that are the same in all the rules, should be defined above, so you don't need to repeat them. If it looks too dirty, you can still group them there (but I guess it's fine).
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.
Some ideas:
params: | ||
sample = config["parameters"]["sample"], | ||
min_qual = config["parameters"]["min_qual"], | ||
min_var_length = config["parameters"]["min_var_length"], |
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.
I agree with @eaasna that all parameters that are the same in all the rules, should be defined above, so you don't need to repeat them. If it looks too dirty, you can still group them there (but I guess it's fine).
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! Re-run the doxygen CI, as it failed temporarily.
…ing_evaluation for a snakemake workflow. Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
4b3eb1c
to
7c72242
Compare
Resolves #123
Our first tool comparison:
CallerComparisonPlot.pdf