Fix bug in process_consequences that was introduced when adding support for VEP without polyphen

[jkgoodrich]

This adds the following fixes to the current process_consequences:

• There are some things wrong with this part of the code:

    modifier = _csq_score(tc)
    flag_condition = (tc.lof == "HC") & (tc.lof_flags != "")
    modifier -= hl.if_else(flag_condition, flag_score, no_flag_score)
    modifier -= hl.if_else(tc.lof == "OS", 20, 0)
    modifier -= hl.if_else(tc.lof == "LC", 10, 0)
  

• I don't think it is supposed to have modifier = _csq_score(tc) since it's already handled by:

    tcl = tcl.map(
        lambda tc: tc.annotate(csq_score=_csq_score(tc) - _csq_score_modifier(tc))
    )
  

• Missingness of .lof and .lof_flags is not handled correctly
  • One problem is the same as the one mentioned for the original code
  • Another problem is that if .lof or .lof_flags is missing, flag_condition will be missing, and therefore csq_score will be missing.
  • Also, if (tc.lof == "HC") is False, then flag_condition evaluates to False, so the no_flag_score will be subtracted from the modifier. This should only happen if (tc.lof == "HC") is True.

The updated function also has the following fixes to the original code (before the support for no polyphen was added):

• Allow lof_flags to be missing in addition to lof_flags == "" for lof == "HC" to have no flag penalty (note that this is a change from previous loftee annotations)
• Pass csq_order to add_most_severe_consequence_to_consequence (in the default case this wouldn't have caused an issue)

Here are some tests showing the comparison of the original code, the current code, and the code in this PR

fixes_to_process_consequences_small.html.zip

[mike-w-wilson]

I had just a couple of comments on the readability of this where we are focusing on the module now. I also realized process_consequences produces a lot of data since we keep the entire struct for all the worst...by...s. Not for this PR but I feel we could add an option to trim this down so say worst_csq_by_gene only returns could be a dict of gene symbol and csq term. I'll make a ticket for the backlog is this is something you agree could be useful?

[mike-w-wilson]

I know this is unchanged from before but since no_flag and flag are only used once and you cant pass a value for flag, I think its more difficult to read with the variables, because you need to go back up to the assignment. I'd just do 500, 500 /(1 + penalize_flags). IF you agree with the value adjustment above.

[jkgoodrich]

just going to leave as is for now since my next PR will completely remove the use of the scores

[mike-w-wilson]

Also I know this is from before but since were in the process of revamping this module, I think the language around and then how we handle the logic in relation to those flags is counterintuitive, i.e the penalize_flag logic is actually a no_flag booster. These values seem arbitrary and I cant imagine anyone is actually using these score values themselves since LOF is deducted so far down. It would be more clear if the scores were no_flag=500, flag = 500/(1+penalize_flags).

[jkgoodrich]

Yeah, I'm actually completely removing the scores in my next PR

[jkgoodrich]

Yeah, my next PR completely changes this function, but it will take more time to review, so just getting this in with minimal changes to fix the bug. These changes were my first round of changes to fix the function before I got completely confused by this and other functions in vep.py and completely rewrote it

[mike-w-wilson]

[mike-w-wilson]

Thank you!