added ReferenceMapping for Genotype, filterByOverlappingRegion for GenotypeRDDFunctions

[erictu]

No description provided.

[AmplabJenkins]

Can one of the admins verify this patch?

[fnothaft]

Jenkins, add to whitelist.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/386/
Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/387/
Test PASSed.

[fnothaft]

Thanks @erictu! This looks great. Can you squash down to a single commit?

[massie]

Seeing this chain of getters makes me wonder if we should protect against null values? Using Option() and orNull?

[fnothaft]

I think the general direction we're moving in (e.g., #469) is to require the API caller to provide input with valid mappings.

From a practical standpoint though, variants are meaningless without being anchored to a reference position. If you get to the point where you have variants without mappings attached to them, you've done something wrong.

[massie]

For example, since you use the variant three times, you can define it once using...

val variant = Option(rec.getVariant)

And then use pattern matching or a simple conditional to skip genotypes without a variant

[fnothaft]

Genotypes without a variant are ill formed... IMO, we shouldn't check for that case. If we did want to validate that genotypes had a variant attached, it should only be via an assert.

[fnothaft]

Also, variants are not nullable in the genotype schema:

record Genotype {
  /**
   The variant called at this site.
   */
  Variant variant;

https://github.com/bigdatagenomics/bdg-formats/pull/39/files changes this, although it is arguable as to whether that is a desirable change (desirable because it makes everything nullable, undesirable because genotypes without variants are meaningless).

[massie]

Would you ever want to project a Genotype without the variant? If so, then you need to make it nullable. If not, then we don't need to worry about the null check here as you say.

[fnothaft]

If so, then you need to make it nullable. If not, then we don't need to worry about the null check here as you say.

Since it isn't nullable now, no reason to do the check.

Would you ever want to project a Genotype without the variant?

Let's discuss this on the PR that makes the change. Conceivably, you could, but not sure if it makes sense to allow people to.

[massie]

+1

-Matt

On Sun, Nov 9, 2014 at 9:38 PM, Frank Austin Nothaft <
notifications@github.com> wrote:

In
adam-core/src/main/scala/org/bdgenomics/adam/rdd/variation/VariationRDDFunctions.scala:

@@ -101,4 +102,12 @@ class GenotypeRDDFunctions(rdd: RDD[Genotype]) extends Serializable with Logging

 bySample

}
+

• def filterByOverlappingRegion(query: ReferenceRegion): RDD[Genotype] = {
• def overlapsQuery(rec: Genotype): Boolean =

•  rec.getVariant.getContig.getContigName.toString == query.referenceName &&
  

If so, then you need to make it nullable. If not, then we don't need to
worry about the null check here as you say.

Since it isn't nullable now, no reason to do the check.

Would you ever want to project a Genotype without the variant?

Let's discuss this on the PR
https://github.com/bigdatagenomics/bdg-formats/pull/39/files that makes
the change. Conceivably, you could, but not sure if it makes sense to
allow people to.

—
Reply to this email directly or view it on GitHub
https://github.com/bigdatagenomics/adam/pull/470/files#r20064839.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/388/
Test PASSed.

[massie]

If you squash this down to a single commit and rebase on master, I think we're ready to merge this.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/401/
Test PASSed.

[fnothaft]

Merged! Thanks @erictu!