Develop#817
Conversation
…lly handled by VV ocde and not vv_hgvs
Issue 815
|
@John-F-Wagstaff recommend merging this then bringing in the VV 4.0.0 coede changes |
|
@John-F-Wagstaff Can you give this a quick sanity check please. Seems to be all OK. All tests passing and additional tests included to cover a wide range of Selenon Protein Variants |
| assert pro_inv_info['edit_start'] != pro_inv_info['edit_end'] | ||
| from_aa = prot_ref_seq[pro_inv_info['edit_start']] | ||
| to_aa = prot_ref_seq[pro_inv_info['edit_end']] | ||
| if posedit: |
There was a problem hiding this comment.
For future code, you could have returned early here, doing so would have avoided all the later indenting that inflated the change log.
It is also best to do so whenever you can as it simplifies the later logic, and makes things more readable. Not only does it make it clear when you have finished but python's use of indenting to handle code blocks also becomes less and less readable the more indented code becomes.
Fixing this now would probably involve re-building the pull request, which I can do if you want, but is not necessary if you don't want to. I am mostly mentioning this as you seem very reluctant in your other code to return early at all, when you probably want to return early as a normal way of handling things (e.g. cases like this, simple error/null input cases ect.).
There was a problem hiding this comment.
Happy for you to refactor it, and yes, I will take the lesson and look to return earlier. Thanks for the nidge. Are you OK to do it? Might as well keep it clean where possible.
There was a problem hiding this comment.
I should be OK to do it, I will have a try.
There was a problem hiding this comment.
Let me know if you get stuck and I can assist too. Thanks
There was a problem hiding this comment.
It looks OK to do, but I will have to do a force push on develop to do this. It would replace all commits from the merge commit for the #815 branch onwards, with commits based on a fixed-up version of the merge commit. This is doable but is is often seen as rude as it can impact other users/branches.
Given how few other people directly get our code from git, let alone from develop this might be OK still, but it is only sensible if you don't have anything based on commits after the merge. The tests seem OK so far, but is this alright by you? (and if so do you have anything based on dev post the #815 merge.)
There was a problem hiding this comment.
@John-F-Wagstaff let me know when this is good to merge. I will do nothing until you say go :).
There was a problem hiding this comment.
If you want to merge into dev rather than wait go ahead, I ran into some problems with tests not getting started with test_, and so not being run by pytest, that need fixing before I complete this. Unless you are messing with vvMixinInit it should only slightly increase the time to finish the fixup. Any branches based on commits post
commit: dc0bfa9 (fix uncertain position parsing of extracted variants errors)
are probably best merged and cherry-picked etc by me while I do the rest. Especially as you will want to not have anything active based on such commits when or after I do the force push.
There was a problem hiding this comment.
no rush. I would rather wait until refactored. I have nothing to push to dev right now. Will you be making the tweaks we identified here as part of a larger push to develop (which includes moving the VCF shorthand code movement of location and other stuff we discussed) or in parts, i.e. the refactoring of this then the additional parts. Just planning when to address the remaining parts of #799 (comment) and making sure I dont update some thing that will clash. Would rather wait for you to say go and what to do next i.e. when I can apply the changes. there is no rush at all.
There was a problem hiding this comment.
OK.
I was just planning on getting the test issues fixed as they impact the Selenon code, then pushing this fix as is, the less I mess with things by re-basing on an non-private branch the better. I may even skip some of the test fixes until after.
There was a problem hiding this comment.
Sounds good, so push the selenon stuff in this commit with the test fixtures working. Do you want to include the edits to the VV VCF shrinking output in your work then, and I will plug it into the web interface? The model is shown in the link. We also want to move the point where VCF shrinking happens from 50bp to 500.
Model for VV dict is
"vcf": {
"alt": "DEL",
"chr": "chr1",
"pos": "1000000-1005000",
"ref": "N"
}
I think the pos in the dict is already a string, but we will have to enforce this behaviour.
John-F-Wagstaff
left a comment
John-F-Wagstaff
left a comment
There was a problem hiding this comment.
OK, this looks good, the only comment I have is more on style, though it would have also made checking which bits of the logic changed easier, so I think this is fine if you want to pull it.
|
@John-F-Wagstaff. While you are refactoring, can you see if there is a way of passing a warning to the users that the variant is in a Seleno Protein encoding gene, so the codon |
2 participants
No description provided.