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

[hail][wip] Local ld prune ptype #6425

Merged
merged 19 commits into from Jul 18, 2019
Merged

Conversation

akotlar
Copy link
Contributor

@akotlar akotlar commented Jun 20, 2019

Tests not passing, not sure why yet. Errors are MatchError.

Stacked on #6421, will see if I can unwind

> Task :test
Running test: Test method bitPackedVectorCorrectWhenOffsetNotZero(is.hail.methods.LocalLDPruneSuite)

Gradle suite > Gradle test > is.hail.methods.LocalLDPruneSuite.bitPackedVectorCorrectWhenOffsetNotZero PASSED
Running test: Test method testBitPackUnpack(is.hail.methods.LocalLDPruneSuite)

Gradle suite > Gradle test > is.hail.methods.LocalLDPruneSuite.testBitPackUnpack FAILED
    scala.MatchError at LocalLDPruneSuite.scala:222
Running test: Test method testIsLocallyUncorrelated(is.hail.methods.LocalLDPruneSuite)

Gradle suite > Gradle test > is.hail.methods.LocalLDPruneSuite.testIsLocallyUncorrelated FAILED
    org.apache.spark.SparkException at LocalLDPruneSuite.scala:214
        Caused by: scala.MatchError
Running test: Test method testR2(is.hail.methods.LocalLDPruneSuite)

Gradle suite > Gradle test > is.hail.methods.LocalLDPruneSuite.testR2 FAILED
    scala.MatchError at LocalLDPruneSuite.scala:244
Running test: Test method testRandom(is.hail.methods.LocalLDPruneSuite)

Gradle suite > Gradle test > is.hail.methods.LocalLDPruneSuite.testRandom FAILED
    java.lang.AssertionError at LocalLDPruneSuite.scala:323
        Caused by: scala.MatchError at LocalLDPruneSuite.scala:323

cc @tpoterba

@tpoterba
Copy link
Contributor

tpoterba commented Jul 9, 2019

@akotlar can you rebase and remove stacked tag? Let's get this in

@akotlar
Copy link
Contributor Author

akotlar commented Jul 9, 2019

Yep, would like to issue the infer ptype wip PR first, targeting today

@akotlar akotlar removed the stacked PR label Jul 9, 2019
@akotlar
Copy link
Contributor Author

akotlar commented Jul 9, 2019

rebased. one merge change in object TableValue apply I wasn't sure about (a constructor deleted). tests likely failing. will follow up on both tonight.

@akotlar
Copy link
Contributor Author

akotlar commented Jul 10, 2019

working on it now

@akotlar
Copy link
Contributor Author

akotlar commented Jul 10, 2019

Still not satisfied with my understanding of the landscape (namely why my changes in LDPruneSuite prior to last commit were failing), but passes

Copy link
Contributor

@tpoterba tpoterba left a comment

Choose a reason for hiding this comment

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

small fixes

@@ -357,7 +356,7 @@ case class LocalLDPrune(
it.map { rv =>
region.clear()
rvb.set(region)
rvb.start(tableType.rowType.physicalType)
rvb.start(tableType.canonicalPType)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be bpvType I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

also the canonicalRVDType above should be RVDType(bpvType, Array("locus", "alleles"))

Copy link
Contributor Author

@akotlar akotlar Jul 10, 2019

Choose a reason for hiding this comment

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

I don't yet understand this section of code well, but making the following changes results in an error in the r2 test

(added rvb.addFields(bpvType, rv, fieldIndicesToAdd) , rvdLP.mapPartitions( RVDType(bpvType, Array("locus", "alleles")) )

 val fieldIndicesToAdd = Array("locus", "alleles", "mean", "centered_length_rec")
      .map(field => bpvType.fieldIdx(field))
    val sitesOnly = rvdLP.mapPartitions(
      RVDType(bpvType, Array("locus", "alleles"))
    )({ it =>
      val region = Region()
      val rvb = new RegionValueBuilder(region)
      val newRV = RegionValue(region)

      it.map { rv =>
        region.clear()
        rvb.set(region)
        rvb.start(bpvType)
        rvb.startStruct()
        rvb.addFields(bpvType, rv, fieldIndicesToAdd)
        rvb.endStruct()
        newRV.setOffset(rvb.end())
        newRV
      }
    })
Gradle suite > Gradle test > is.hail.methods.LocalLDPruneSuite.testIsLocallyUncorrelated FAILED
    java.lang.IllegalArgumentException at LocalLDPruneSuite.scala:355
Running test: Test method testR2(is.hail.methods.LocalLDPruneSuite)

Tired, not thinking clearly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will finish digging through tomorrow:

java.lang.IllegalArgumentException: requirement failed
	at scala.Predef$.require(Predef.scala:212)
	at is.hail.expr.ir.TableValue.<init>(TableValue.scala:45)
	at is.hail.methods.LocalLDPrune.execute(LocalLDPrune.scala:368)
	at is.hail.methods.LocalLDPrune$$anonfun$apply$3.apply(LocalLDPrune.scala:286)
	at is.hail.methods.LocalLDPrune$$anonfun$apply$3.apply(LocalLDPrune.scala:284)
	at is.hail.expr.ir.ExecuteContext$$anonfun$scoped$1.apply(ExecuteContext.scala:8)
	at is.hail.expr.ir.ExecuteContext$$anonfun$scoped$1.apply(ExecuteContext.scala:7)
	at is.hail.utils.package$.using(package.scala:596)
	at is.hail.annotations.Region$.scoped(Region.scala:11)
	at is.hail.expr.ir.ExecuteContext$.scoped(ExecuteContext.scala:7)
	at is.hail.methods.LocalLDPrune$.apply(LocalLDPrune.scala:284)
	at is.hail.methods.LocalLDPruneSuite.testIsLocallyUncorrelated(LocalLDPruneSuite.scala:355)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccess

@akotlar
Copy link
Contributor Author

akotlar commented Jul 10, 2019

With this issue I think this PR is ok?

@tpoterba
Copy link
Contributor

needs a bump

@danking danking merged commit 47a7b97 into hail-is:master Jul 18, 2019
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.

None yet

3 participants