Develop

[Peter-J-Freeman]

No description provided.

[Peter-J-Freeman]

@John-F-Wagstaff recommend merging this then bringing in the VV 4.0.0 coede changes

[Peter-J-Freeman]

@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

[John-F-Wagstaff]

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.).

[Peter-J-Freeman]

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.

[John-F-Wagstaff]

I should be OK to do it, I will have a try.

[Peter-J-Freeman]

Let me know if you get stuck and I can assist too. Thanks

[John-F-Wagstaff]

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.)

[Peter-J-Freeman]

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.

[John-F-Wagstaff]

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.

[Peter-J-Freeman]

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]

I have pushed all the changes needed to get this back to where we want it. I will change my current branch of the mapping fixes to match the new VCF format and make pull requests into dev ASAP.

When you try to pull from this git will recommend merging or re-basing, don't!

If you have no local changes the you just need to delete the last few commits from your local branches to get this to pull correctly. The other options will make a lot of mess. Given this it is probably best to delete and pull the new version on all affected copies of develop, otherwise if you use an old version as the basis for new work it will cause issues to untangle it later. If you do have local commits (or branches) based on this contact me and we can work the best method out, though for commits it will probably just want git branch followed handling develop as above, followed by git cherry-pick from the backup branch you just created with the extra changes.

[Peter-J-Freeman]

Done, I deleted my local branch and re-pulled from develop.

[John-F-Wagstaff]

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.

[Peter-J-Freeman]

@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 UGA is being translated to Sec and not Ter. That would get around comments from Johan that there are no warnings. I didnt think to warn for successful validations, but perhaps for educational purposes, this might be useful to our users