Skip to content
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

Add genotype job tests #419

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Add genotype job tests #419

wants to merge 25 commits into from

Conversation

vivbak
Copy link
Contributor

@vivbak vivbak commented Jul 26, 2023

This PR adds the genotype job tests as described https://miro.com/app/board/uXjVPPN9TaM=/

Not every use case was handled as expected, described below

Config Param Value Reason
ref_fasta A non-existent reference file We'd need to either run the hail batch job, OR add file validation to the genotype job
A file of the incorrect format for a reference As above
scatter_count_genotype 0 or negative value According to the testing documentation this should fail. It does not. The current behaviour is to default to 1 if an invalid value is set.
reblock_gq_bands 10 This tests fails, but I don't think it should. Commented out for now. Should we update implementation?
[0,10,20] We'd either need to run the hail batch job, OR add additional validation.
[10,20,500] As above
{10,20,30} This input is not invalid, it will be processed like a list and is passed correctly to the tool

@vivbak vivbak marked this pull request as ready for review August 14, 2023 04:16
@vivbak vivbak marked this pull request as draft August 14, 2023 06:04
@vivbak vivbak marked this pull request as ready for review August 14, 2023 06:08
* Split test cases to avoid if-else statements and improve readability
* Test some additional cases, like exome/genome.
* Use select_jobs to avoid iterating through all the jobs
Copy link
Contributor

@KatalinaBobowik KatalinaBobowik left a comment

Choose a reason for hiding this comment

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

From an analysis perspective, this looks good to me. The only thing I can't really comment on is the GQ band thresholding, and why these intervals are chosen (@silkm not sure if you know more about this?). Otherwise 👍

Copy link
Contributor

@KatalinaBobowik KatalinaBobowik left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@violetbrina violetbrina left a comment

Choose a reason for hiding this comment

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

From the Software side it looks good to me!

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.

None yet

3 participants