[ADAM-461] Fix ReferenceRegion and ReferencePosition impl #469

laserson · 2014-11-07T22:04:24Z

This commit mainly does 3 things:

Fixes impl of ReferenceRegion.width.
Removes Options from ReferencePosition.
Corrected impl of liftOver. However, it now has different semantics: in all cases, the list of regions must be provided in genome order.
Simplifies structure of case classes.

Fixes #461.

laserson · 2014-11-07T22:04:47Z

@tdanford please review closely :)

laserson · 2014-11-07T22:05:49Z

also, having a list of exons for a reverse-strand transcript still makes more sense to me to be in genome-order, and traversing it backwards (from 5' to 3'). What do you think?

fnothaft · 2014-11-07T22:06:10Z

adam-core/src/main/scala/org/bdgenomics/adam/models/ReferencePosition.scala

  }
 }

-case class ReferencePositionWithOrientation(refPos: Option[ReferencePosition],
+case class ReferencePositionWithOrientation(refPos: ReferencePosition,


+1; this shouldn't have been an option.

+1 (did I write this code originally? if so, it was me being even more of a Scala-n00b than I am now.)

fnothaft · 2014-11-07T22:10:03Z

also, having a list of exons for a reverse-strand transcript still makes more sense to me to be in genome-order, and traversing it backwards (from 5' to 3'). What do you think?

Both ways seem reasonable to me. Why do you prefer them to be genome-ordered, instead of transcript-ordered?

fnothaft · 2014-11-07T22:11:35Z

adam-core/src/test/scala/org/bdgenomics/adam/models/IndelTableSuite.scala

@@ -29,10 +29,6 @@ class IndelTableSuite extends SparkFunSuite {
    assert(indelTable.getIndelsInRegion(ReferenceRegion("1", 0L, 2000L)).length === 1)
  }

-  test("check for indels in a contig that doesn't exist") {


Why'd this test get dropped?

What exactly is this testing? Does it make sense to instantiate an empty ReferenceRegion?

The important thing this is testing is that no indels exist on a contig ("0") that doesn't exist. You are correct though; it shouldn't be empty.

laserson · 2014-11-07T22:19:12Z

ReferenceRegion enforces that start < end, so it seems like big/little endian-style confusion. To avoid that, my preference is that all objects are oriented in a way so that if you just glue them together, they are correct. Put another way, it's confusing to traverse the list of exons in order, but the sequence of the exon backwards.

fnothaft · 2014-11-07T22:21:13Z

Put another way, it's confusing to traverse the list of exons in order, but the sequence of the exon backwards.

That's a good TL;DR. I would agree with your point.

AmplabJenkins · 2014-11-07T22:25:00Z

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

laserson · 2014-11-07T22:29:39Z

adam-core/src/test/scala/org/bdgenomics/adam/models/IndelTableSuite.scala

@@ -30,7 +30,7 @@ class IndelTableSuite extends SparkFunSuite {
  }

  test("check for indels in a contig that doesn't exist") {
-    assert(indelTable.getIndelsInRegion(ReferenceRegion("0", 0L, 0L)).length === 0)
+    assert(indelTable.getIndelsInRegion(ReferenceRegion("0", 0L, 1L)).length === 0)


Ok, test is back, and passes now.

Excellent. Thank you for catching the empty region!

AmplabJenkins · 2014-11-07T22:44:14Z

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

tdanford · 2014-11-12T21:36:29Z

adam-core/src/main/scala/org/bdgenomics/adam/models/ReferenceRegion.scala

-   * strand, else it will be on the positive strand.
-   * @return Returns an oriented reference region.
+   * @param referenceName The name of the sequence (chromosome) in the reference genome
+   * @param start The 0-based residue-coordinate for the start of the region


@massie and I were talking about this in IRC, we really need to explain somewhere not in code what our coordinate conventions are. I mean, I like the comments, but we also need non-code versions of the same comments.

Strongly agree with that.

Note, btw, that in my mental model, region.start should always be <
region.end.

Agreed -- the convention of "end < start" to designate negative-strand regions is messed up

tdanford · 2014-11-14T11:41:30Z

I've started a rudimentary non-code writeup of the coordinate convention question here:
https://github.com/bigdatagenomics/adam/wiki/Coordinate-Conventions

Please feel free to (a) edit it, (b) tell me I'm wrong and get me to edit it some more, or (c) adapt it as content in some other location (the docs/ directory, Matt?)

fnothaft · 2014-11-14T17:43:34Z

Please feel free to (a) edit it, (b) tell me I'm wrong and get me to edit it some more, or (c) adapt it as content in some other location (the docs/ directory, Matt?)

I think the docs directory would be the best approach. We're trying to move documentation to there, so that it is autogenerated on each release.

fnothaft · 2014-11-14T17:49:45Z

@laserson can you rebase? Then, I'll merge.

laserson · 2014-11-14T20:19:00Z

Done.

AmplabJenkins · 2014-11-14T20:34:05Z

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

Build result: FAILURE

GitHub pull request #469 of commit 619ec42 automatically merged.[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-slave-01 (centos) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/469/merge^{commit} # timeout=10Checking out Revision 207a1b3277d39d915e80d7e2f49fe5e5979a07f4 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 207a1b3277d39d915e80d7e2f49fe5e5979a07f4 > git rev-list cc683a5 # timeout=10Triggering ADAM-prb » 2.3.0,centosTriggering ADAM-prb » 1.0.4,centosTriggering ADAM-prb » 2.2.0,centosADAM-prb » 2.3.0,centos completed with result FAILUREADAM-prb » 1.0.4,centos completed with result FAILUREADAM-prb » 2.2.0,centos completed with result FAILURE
Test FAILed.

fnothaft · 2014-11-14T20:35:38Z

Can you run the format-source script?

laserson · 2014-11-14T22:57:20Z

Done.

AmplabJenkins · 2014-11-14T23:28:40Z

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

Build result: ABORTED

GitHub pull request #469 of commit 9b8c7b0 automatically merged.[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-slave-01 (centos) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/469/merge^{commit} # timeout=10Checking out Revision ba61a5e (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f ba61a5e > git rev-list cc683a5 # timeout=10Triggering ADAM-prb » 2.3.0,centosTriggering ADAM-prb » 1.0.4,centosTriggering ADAM-prb » 2.2.0,centosADAM-prb » 2.3.0,centos completed with result ABORTEDADAM-prb » 1.0.4,centos completed with result ABORTEDADAM-prb » 2.2.0,centos completed with result ABORTED
Test FAILed.

fnothaft · 2014-11-14T23:43:13Z

Jenkins, retest this please.

AmplabJenkins · 2014-11-15T00:18:30Z

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

Build result: ABORTED

GitHub pull request #469 of commit 9b8c7b0 automatically merged.[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-slave-01 (centos) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/469/merge^{commit} # timeout=10Checking out Revision ba61a5e (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f ba61a5e > git rev-list cc683a5 # timeout=10Triggering ADAM-prb » 2.3.0,centosTriggering ADAM-prb » 1.0.4,centosTriggering ADAM-prb » 2.2.0,centosADAM-prb » 2.3.0,centos completed with result ABORTEDADAM-prb » 1.0.4,centos completed with result ABORTEDADAM-prb » 2.2.0,centos completed with result ABORTED
Test FAILed.

fnothaft · 2014-11-15T00:21:24Z

Jenkins, retest this please.

AmplabJenkins · 2014-11-15T00:53:21Z

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

Build result: ABORTED

GitHub pull request #469 of commit 9b8c7b0 automatically merged.[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-slave-01 (centos) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/469/merge^{commit} # timeout=10Checking out Revision ba61a5e (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f ba61a5e > git rev-list cc683a5 # timeout=10Triggering ADAM-prb » 2.3.0,centosTriggering ADAM-prb » 1.0.4,centosTriggering ADAM-prb » 2.2.0,centosADAM-prb » 2.3.0,centos completed with result ABORTEDADAM-prb » 1.0.4,centos completed with result ABORTEDADAM-prb » 2.2.0,centos completed with result ABORTED
Test FAILed.

fnothaft · 2014-11-15T00:55:26Z

@sknapp Can you take a look? This build has timed out several times. I'll test on my local machine to see if there's a problem with this changeset.

laserson · 2014-11-16T01:51:31Z

Builds fine on my local machine.

laserson · 2014-11-17T01:08:05Z

If it's come to building locally :( any chance we can get this in as well?

fnothaft · 2014-11-17T01:27:11Z

OK, I've upped the build timeout. Can you rebase this?

laserson · 2014-11-17T01:35:37Z

Rebased.

AmplabJenkins · 2014-11-17T02:23:47Z

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

Build result: ABORTED

GitHub pull request #469 of commit 4ec66c7 automatically merged.[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-slave-01 (centos) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/469/merge^{commit} # timeout=10Checking out Revision 28dc0d1063cc42a77951c19afe5841252369016e (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 28dc0d1063cc42a77951c19afe5841252369016e > git rev-list f211b54 # timeout=10Triggering ADAM-prb » 2.3.0,centosTriggering ADAM-prb » 1.0.4,centosTriggering ADAM-prb » 2.2.0,centosADAM-prb » 2.3.0,centos completed with result ABORTEDADAM-prb » 1.0.4,centos completed with result ABORTEDADAM-prb » 2.2.0,centos completed with result ABORTED
Test FAILed.

fnothaft · 2014-11-17T02:30:51Z

Jenkins, retest this please.

AmplabJenkins · 2014-11-17T03:03:37Z

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

Build result: FAILURE

GitHub pull request #469 of commit 4ec66c7 automatically merged.[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-slave-01 (centos) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/469/merge^{commit} # timeout=10Checking out Revision 28dc0d1063cc42a77951c19afe5841252369016e (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 28dc0d1063cc42a77951c19afe5841252369016e > git rev-list f211b54 # timeout=10Triggering ADAM-prb » 2.3.0,centosTriggering ADAM-prb » 1.0.4,centosTriggering ADAM-prb » 2.2.0,centosADAM-prb » 2.3.0,centos completed with result FAILUREADAM-prb » 1.0.4,centos completed with result FAILUREADAM-prb » 2.2.0,centos completed with result FAILURE
Test FAILed.

fnothaft · 2014-11-17T03:51:14Z

@laserson can you run scripts/format-source?

This commit mainly does 3 things: 1. Fixes impl of `ReferenceRegion.width`. 2. Removes `Option`s from `ReferencePosition`. 3. Corrected impl of liftOver. However, it now has different semantics: in all cases, the list of regions must be provided in genome order. 4. Simplifies structure of case classes. Fixes bigdatagenomics#461.

laserson · 2014-11-17T04:11:01Z

argh! sorry...fixed it

AmplabJenkins · 2014-11-17T04:29:49Z

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

[ADAM-461] Fix ReferenceRegion and ReferencePosition impl

fnothaft · 2014-11-17T04:30:35Z

Merged! Thanks @laserson, and thanks for the patience with the build timeouts...

fnothaft reviewed Nov 7, 2014
View reviewed changes

laserson force-pushed the ADAM-461 branch from 92de88b to 58fcf63 Compare November 7, 2014 22:27

laserson reviewed Nov 7, 2014
View reviewed changes

fnothaft mentioned this pull request Nov 10, 2014

added ReferenceMapping for Genotype, filterByOverlappingRegion for GenotypeRDDFunctions #470

Merged

tdanford reviewed Nov 12, 2014
View reviewed changes

laserson force-pushed the ADAM-461 branch from 58fcf63 to 619ec42 Compare November 14, 2014 20:17

laserson force-pushed the ADAM-461 branch from 619ec42 to 9b8c7b0 Compare November 14, 2014 22:56

laserson force-pushed the ADAM-461 branch from 9b8c7b0 to 4ec66c7 Compare November 17, 2014 01:35

laserson force-pushed the ADAM-461 branch from 4ec66c7 to 18dcdcd Compare November 17, 2014 04:10

fnothaft added a commit that referenced this pull request Nov 17, 2014

Merge pull request #469 from laserson/ADAM-461

51610d1

[ADAM-461] Fix ReferenceRegion and ReferencePosition impl

fnothaft merged commit 51610d1 into bigdatagenomics:master Nov 17, 2014

laserson deleted the ADAM-461 branch December 2, 2014 03:39

[ADAM-461] Fix ReferenceRegion and ReferencePosition impl #469

[ADAM-461] Fix ReferenceRegion and ReferencePosition impl #469

Conversation

laserson commented Nov 7, 2014

laserson commented Nov 7, 2014

laserson commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnothaft commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laserson commented Nov 7, 2014

fnothaft commented Nov 7, 2014

AmplabJenkins commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdanford commented Nov 14, 2014

fnothaft commented Nov 14, 2014

fnothaft commented Nov 14, 2014

laserson commented Nov 14, 2014

AmplabJenkins commented Nov 14, 2014

Build result: FAILURE

fnothaft commented Nov 14, 2014

laserson commented Nov 14, 2014

AmplabJenkins commented Nov 14, 2014

Build result: ABORTED

fnothaft commented Nov 14, 2014

AmplabJenkins commented Nov 15, 2014

Build result: ABORTED

fnothaft commented Nov 15, 2014

AmplabJenkins commented Nov 15, 2014

Build result: ABORTED

fnothaft commented Nov 15, 2014

laserson commented Nov 16, 2014

laserson commented Nov 17, 2014

fnothaft commented Nov 17, 2014

laserson commented Nov 17, 2014

AmplabJenkins commented Nov 17, 2014

Build result: ABORTED

fnothaft commented Nov 17, 2014

AmplabJenkins commented Nov 17, 2014

Build result: FAILURE

fnothaft commented Nov 17, 2014

laserson commented Nov 17, 2014

AmplabJenkins commented Nov 17, 2014

fnothaft commented Nov 17, 2014