Skip to content
This repository has been archived by the owner on Jan 24, 2018. It is now read-only.

Fix for #1334: Refactor transcript annotation #1562

Merged
merged 1 commit into from
Feb 20, 2017

Conversation

andrewjesaitis
Copy link
Member

  • Places extra data in attributes field
  • Simplify code path for annotation parsing

Copy link
Member

@david4096 david4096 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removes code and adds tests. 💯

@@ -1040,6 +1057,33 @@ class HtslibVariantAnnotationSet(AbstractVariantAnnotationSet):
Class representing a single variant annotation derived from an
annotated variant set.
"""
CSQ_FIELDS = ("alternate_bases", "gene", "feature_id",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I aspire to have these tuples as part of the registry editor.

It should be possible to easily write a VCF->ANN plugin that accepts a tuple or dictionary that can effectively parse a ANN field.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought the same thing since we are mixing data with code here. But, we'll save it for an annotation plugin refactor.

return reduce(getattr, path.split('.'), obj)


def deepSetAttr(obj, path, val):
Copy link
Member

@david4096 david4096 Feb 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could probably go into protocol in schemas. I think this pattern of dot access for dictionaries is a sane way to work with data when you don't want to make a new class in python, and mocks the protocol buffers and peewee models well.

This is what I did for the peer service

class Struct:
    def __init__(self, **entries):
        self.__dict__.update(entries)

###

map(lambda x: Struct(**x), self._peers[offset:offset + limit])

Maybe you could do this recursively?

https://github.com/ga4gh/server/pull/1556/files#diff-02d20abc7e4d68c8ae177064ca45176aR49

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I'm moving these over to schemas then.

Here's the recursive version:

def deepGetAttr(obj, path):
    first, sep, rest = path.partition(".")
    obj = getattr(obj, first)
    if rest:
        obj = deepGetAttr(obj, rest)
    return obj

Seems like the recursive version is a little bit uglier. Any specific benefit you were thinking of?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, failed to communicate myself properly how you've approached it is fine. Sending in dot notation as a string scares me a little bit :).

I was thinking something like this, but don't let it hold up this PR.

@andrewjesaitis
Copy link
Member Author

Also, how does it work when a PR in server depends on a PR in schemas? Should I commit my constraints.txt file that points to my version of schemas that has he edited protocol.py?

@dcolligan
Copy link
Member

Here's how I would advise doing it:

  • push a branch to ga4gh/schemas and open a PR on it
  • open a PR with your contstraints.txt file line for schemas pointing to the remote schemas branch
  • presumably both the schemas and the server changes are going in, and the server depends on schemas, so merge the schemas PR first, once it is approved
  • change your server PR to use the default version of constraints.txt (that is, the one that points to all the master branches)
  • merge the server PR

@david4096
Copy link
Member

Hey we'll merge this next @andrewjesaitis !

* Place extra data in attributes field
* Simplify code path for annotation parsing
@codecov-io
Copy link

Codecov Report

Merging #1562 into master will decrease coverage by -0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
- Coverage   85.61%   85.59%   -0.02%     
==========================================
  Files          33       33              
  Lines        7188     7152      -36     
  Branches      898      894       -4     
==========================================
- Hits         6154     6122      -32     
+ Misses        851      848       -3     
+ Partials      183      182       -1
Impacted Files Coverage Δ
ga4gh/server/datamodel/variants.py 94.27% <100%> (+0.26%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdfd148...5792e83. Read the comment docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants