Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added clarifying note to ADAMVariantContext #274

Merged
merged 1 commit into from
Jun 26, 2014

Conversation

laserson
Copy link
Contributor

Not to be confused with ADAMVariationContext.

This is probably not the right place to put this note. Where should it go?

@fnothaft
Copy link
Member

+1, good call!

@fnothaft
Copy link
Member

BTW, I think that is an OK place to put the note, but I would wrap it in JAVADOC style comments (/*.../) so that it gets rendered as part of the Scaladoc for the API. Alternatively, you could add it as:

/**
 * @see org.bdgenomics.adam.rdd.variation.ADAMVariationContext This class is related to...
 */

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@fnothaft
Copy link
Member

Jenkins, add to whitelist and test this please.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/21/

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/23/

@heuermh
Copy link
Member

heuermh commented Jun 24, 2014

I would be in favor of renaming org.bdgenomics.adam.models.ADAMVariantContext to something else entirely.

@tdanford
Copy link
Contributor

+1 on Michael's comment -- we had talked a while ago about renaming ADAMRecord too, I don't think there's any reason to stick to a naming scheme which matches GATK or Picard.

@fnothaft
Copy link
Member

I actually don't think we need the ADAMVariantContext anymore anyways; it was useful with the original flat variant and genotypes schema that we had, but is no longer necessary with the current nested genotype schema.

@arahuja @timodonnel would dropping the ADAMVariantContext cause problems for guacamole?

@massie
Copy link
Member

massie commented Jun 24, 2014

+1 on removing it if it's not needed. @arahuja @timodonnel?

@massie
Copy link
Member

massie commented Jun 25, 2014

Jenkins, test this please.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/40/

@massie
Copy link
Member

massie commented Jun 25, 2014

Jenkins test this please

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/46/

@tdanford
Copy link
Contributor

No word from Arun or Tim -- why don't we merge this, and open a discussion issue for removing ADAMVariantContext. All in favor?

@tdanford
Copy link
Contributor

Unless there are objections, Uri if you rebase this I will merge it.

@fnothaft
Copy link
Member

@tdanford +1, we should do the discussion on the list anyways.

@laserson
Copy link
Contributor Author

Rebased.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/50/

fnothaft added a commit that referenced this pull request Jun 26, 2014
Added clarifying note to `ADAMVariantContext`
@fnothaft fnothaft merged commit 94551a0 into bigdatagenomics:master Jun 26, 2014
@fnothaft
Copy link
Member

Merged! Thanks @laserson!

@laserson laserson deleted the var-ctx-note branch August 13, 2014 17:56
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.

6 participants