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
TopiaryPredictor #81
TopiaryPredictor #81
Conversation
…having to pass very large number of arguments
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.
mostly LGTM, couple minor things
topiary/predictor.py
Outdated
expressed above this threshold. | ||
|
||
transcript_expression_threshold : float, optional | ||
If transcript_expression_dict is given, only keep effects on |
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.
Is it possible to rephrase this docstring so that you don't need to search for what transcript_expression_dict
is?
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.
Slightly rephrased
|
||
padding_around_mutation : int | ||
How many residues surrounding a mutation to consider including in a | ||
candidate epitope. Default is the minimum size necessary for epitope |
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.
where is this default set? In the constructor you just keep the passed-in value
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.
Clarified the default by moving check_padding_around_mutation
call to __init__
.
topiary/predictor.py
Outdated
---------- | ||
effects : Varcode.EffectCollection | ||
|
||
padding_around_mutation : int |
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 is no longer a param 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.
Deleted
We currently have two top-level functions called
predict_epitopes_from_variants
andpredict_epitopes_from_effects
with a very large number of arguments. Creating another top-level function (e.g. to predict from protein changes without genomic variants) would require duplicating a lot of thing long argument list. To make it easier to add features in the future I'm making aTopiaryPredictor
object which holds properties shared between different top-level methods.This change is