Moved VariantEffects so it's using the Enum #78
Conversation
There was a problem hiding this comment.
Hi @lnrekerle I think this is great!
I pushed a few little tweaks and there is one more thing that I'd like to ask you to do. Please see the comment below, regarding handling missing VariantEffect enum members.
| con = "FIVE_PRIME_UTR_VARIANT" | ||
| if con[0] == '3': | ||
| con = 'THREE_PRIME_UTR_VARIANT' | ||
| var_effects.append(VariantEffect[con.upper()]) |
There was a problem hiding this comment.
I think you should make the line #100 a little more verbose and add code to handle failure. What if VEP returns a value that does not have a corresponding enum member? I think we should log a warning and ask the user to report missing variant effect to developers (make a Github issue), so that we can add it.
I think you should put the logic for decoding str -> VariantEffect into a separate private method, e.g.
def _parse_variant_effect(self, effect: str) -> typing.Optional[VariantEffect]:
# TODO implement
passBasically, the code that you have in the loop should be the method's body. Then, the loop applies the method and appends the variant effect to var_effects if it is not None. The warning can be made in the method's body.
There was a problem hiding this comment.
@ielis I have made this change, let me know if this is what you were thinking. I'll merge the changes once I get the okay!
There was a problem hiding this comment.
So, as far as I can see, 5 and 3 are the only numbers we can encounter in variant effect names. This is because the "direction" of DNA depends on moieties bound to 3' and 5' carbons in ribose. So, correct me if I'm wrong, but I think these are the only numbers we can reasonably expect to get in the context of VariantEffect and, therefore, having digit_word_map and replacing 0 with ZERO seems reeeally odd..
I think we should handle this with two explicit if/else tests:
if effect == '5_PRIME_UTR_VARIANT':
pass
elif effect == '3_PRIME_UTR_VARIANT':
pass
else:
# use VariantEffect[effect.upper()] and log warning if things go wrong.
passBTW, self._logging is not callable.
There was a problem hiding this comment.
Hi @lnrekerle
I'd like to ask you to do 2 more updates, see the comment below. After that, I think this is ready to go.
| @@ -80,6 +80,18 @@ def annotate(self, variant_coordinates: VariantCoordinates) -> typing.Sequence[T | |||
|
|
|||
| return annotations | |||
|
|
|||
| def _parse_variant_effect(self, effect: str) -> typing.Optional[VariantEffect]: | |||
| if effect.upper() == "5_PRIME_UTR_VARIANT": | |||
There was a problem hiding this comment.
I have two minor suggestions:
- please refactor such that you do the conversion to uppercase at most once per method body instead of 3 times in the worst case.
- logging, as currently written, will construct the string unconditionally, regardless of the set log level threshold (e.g. even if the user sets the threshold to see ERRORs only). This is because you're submitting the f-string which should be avoided when logging a statement. Instead, we should do something like this:
Using this form, the logging framework will only create the final string if the logging threshold is low enough (WARN or lower in this case).
self._logging.warning('Variant effect %s was not found ...', effect)
No description provided.