-
Notifications
You must be signed in to change notification settings - Fork 19
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 parameters for penalty and threshold #138
Add parameters for penalty and threshold #138
Conversation
Parameters are tested such that they respond. First no penalty is applied, yielding score -6 (which matches the RankResult). Then a penalty of 5 is applied, changing the score to -6. Finally, the threshold for penalty is lowered below the low scoring compound, thus avoiding the penalty by having a passing compound variant. Score going back to -6.
|
@@ -45,8 +45,10 @@ | |||
is_flag=True, | |||
help='If variants are annotated with the Variant Effect Predictor.' | |||
) | |||
@click.option('--threshold', type=int, help="Threshold for model-dependent penalty if no compounds with passing score", default=9) |
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.
Feel very free to help me improve these help texts
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.
Correctly spelling threshold is already a strong text! 😸
Not sure how to turn to with reviewing here. Maybe @dnil , @northwestwitch or @torbjorgen could give this a look? |
Thanks for the review @torbjorgen ! I don't have merge-access to the repo. Maybe someone else could help me merge this PR? |
I don't have access to this repo, so I am unable to merge / release. Not sure who to ask. Could maybe @torbjorgen or @dnil help me out here? With merging or giving me access to the repo so I can do it myself. |
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.
This looks safe enough - defaults are exactly as before. The PR exposes a previously somewhat internally hidden, and rather important aspect of the model.
@@ -45,8 +45,10 @@ | |||
is_flag=True, | |||
help='If variants are annotated with the Variant Effect Predictor.' | |||
) | |||
@click.option('--threshold', type=int, help="Threshold for model-dependent penalty if no compounds with passing score", default=9) |
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.
Correctly spelling threshold is already a strong text! 😸
@click.pass_context | ||
def compound(context, variant_file, silent, outfile, vep, processes, temp_dir): | ||
def compound(context, variant_file, silent, outfile, vep, threshold: int, penalty: int, processes, temp_dir): |
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 would call this a minor, since it adds functionality in a completely backwards compatible manner. It does not correct a bug per se, but almost. ;) |
Thanks for the review 🙏 |
* Add parameters for penalty and threshold * Cleaning things up * Cleaning up and changelog
Description
This addresses two issues in Lund with using Genmod compounds.
--penalty 0
here).Close #137
Further discussed with @dnil and @northwestwitch in: Clinical-Genomics/scout#4823
Added
Changed
Fixed
How to prepare for test
us
paxa
How to test
Expected test outcome
Review
Thanks for filling in who performed the code review and the test!
This version is a
Implementation Plan