-
Notifications
You must be signed in to change notification settings - Fork 7
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
Run qc on samples #102
Run qc on samples #102
Conversation
Also rename some confusing functions
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.
LGTM modulo 1 question and 1 stylistic remark :)
let fastqc_pipeline ~normal_samples ~tumor_samples ?rna_samples () = | ||
let normal_fastqcs = | ||
List.mapi | ||
~f:(fun i fq -> qc fq |> Bfx.save (sprintf "QC:normal-%d" i)) |
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.
Just a question, the FASTQ samples, don't they have different names that we could use there?
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.
Filed hammerlab/biokepi#387
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.
👍
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.
Agreed this would be superior, also filing in Epidisco #104
?rna_results ~parameters ~normal_bam_flagstat ~tumor_bam_flagstat ~fastqcs | ||
= | ||
let rna_bam_flagstat = | ||
rna_results >>= fun {rna_bam_flagstat; _} -> return rna_bam_flagstat in |
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 open Option
is too far up,
Optionis not meant to be opened globally (Esp. since this case is actually an
Option.maprather than
bind`).
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.
Seems reasonable.
Why is this an Option.map instead of/just as much a bind?
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.
Option.map
shows that None
stays None
, and Some _
stays Some _
→ less mental burden for the reader (?).
bind
is more powerful in a way, so overkill 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.
Good point 👍
@smondet back to you for a final look (I'm also reading over again to make sure this is sane) |
@ihodes LGTM! |
fixes #101