Skip to content

Associate EnsemblRelease with each Variant object#20

Merged
iskandr merged 6 commits intomasterfrom
variants_know_about_reference
Feb 23, 2015
Merged

Associate EnsemblRelease with each Variant object#20
iskandr merged 6 commits intomasterfrom
variants_know_about_reference

Conversation

@iskandr
Copy link
Copy Markdown
Contributor

@iskandr iskandr commented Feb 20, 2015

  • every Variant must be constructed with an explicit EnsemblRelease (this is mildly annoying/inelegant but simplifies a lot of other code)
  • moved a lot of the transcript effect inference from core_logic onto Variant
  • renamed VariantEffect to Annotation
  • added a Variant.annotate() method, which replaces the need for VariantAnnotator
  • got rid of the generic load_variants function, now call load_vcf or load_maf explicitly
  • since each Variant carries its own EnsemblRelease, it's now possible for variants from multiple references to be in the same VariantCollection. I've seen a TCGA-derived MAF which mixes NCBI builds, so this might be desirable (it also gives meaning to taking the union of two variant collections aligned to different references). However, the potential for confusion here is suspicious.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd suggest renaming this default_ensembl

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you say more about why that's a better name?
On Feb 20, 2015 11:35 AM, "timodonnell" notifications@github.com wrote:

In test/test_cosmic_mutations.py
#20 (comment):

@@ -9,11 +10,11 @@
FrameShiftTruncation,
)

-annot = VariantAnnotator(75)
+ensembl = EnsemblRelease(75)

I'd suggest renaming this default_ensembl


Reply to this email directly or view it on GitHub
https://github.com/hammerlab/varcode/pull/20/files#r25082212.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's used only as a default parameter, right? Otherwise it looks to me like we're declaring that this module uses a particular ensembl release

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah wait sorry, nvm. somehow didn't realize this was in a test method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

@timodonnell
Copy link
Copy Markdown
Contributor

Well hope you don't mind the pile of comments, but besides all that, LGTM

… from common to string_helpers, renamed transcript_mutation_effects to effect
iskandr added a commit that referenced this pull request Feb 23, 2015
Associate EnsemblRelease with each Variant object
@iskandr iskandr merged commit 37e3aad into master Feb 23, 2015
@iskandr iskandr deleted the variants_know_about_reference branch February 23, 2015 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants