Reference Genome #3 #2086
Reference Genome #3 #2086
Conversation
jigold
commented
Aug 9, 2017
jigold
commented
- Added genome reference to TVariant, TInterval, TLocus
| @@ -62,20 +63,20 @@ trait HailRepFunctions { | |||
| def typ = TGenotype | |||
| } | |||
|
|
|||
| implicit object variantHr extends HailRep[Variant] { | |||
| def typ = TVariant | |||
| implicit def variantHr[T](implicit hrt: HailRep[T]) = new HailRep[Variant] { | |||
There was a problem hiding this comment.
This shouldn't have a HailRep as a parameter, since it isn't parameterized by a Type but by a GenomeReference. There should be a new class that mirrors TVariable but isn't in the type hierarchy. Something like GRVariable which matches a reference with unification. It should be a GenomeReference so you can construct a TVariant from one.
|
@cseed Back to you. |
|
|
||
| override def toString = "GenomeReference" | ||
|
|
||
| def scalaClassTag: ClassTag[GenomeReference] = classTag[GenomeReference] |
There was a problem hiding this comment.
I think you can delete this (required by type).
| @@ -103,3 +109,29 @@ object GenomeReference { | |||
| } yield GenomeReference(name, contigs, Set(xContig), Set(yContig), Set(mtContig), parX ++ parY) | |||
| } | |||
|
|
|||
| case class GRVariable(gr: GenomeReference, var t: Type = null) extends Serializable { | |||
There was a problem hiding this comment.
So this should have var gr: GenomeReference = null and no type field.
There was a problem hiding this comment.
It should extend GenomeReference. Deciding the right way to do this might be a bit tricky. Then TVariant.unify, etc., should unify the genome reference variable. Let me know if this doesn't make sense.
|
@cseed Back to you. Is there a good way to test if it's actually working? |
jigold
force-pushed
the
refgenome_v2_3
branch
2 times, most recently
from
a582d31
to
0683d34
Compare
- Added genome reference to TVariant, TInterval, TLocus
| @@ -55,9 +55,9 @@ object Annotation { | |||
| null | |||
| else | |||
| t match { | |||
| case TVariant => a.asInstanceOf[Variant].toRow | |||
| case x: TVariant => a.asInstanceOf[Variant].toRow | |||
There was a problem hiding this comment.
_ is traditional if you're not using the variable.
| @@ -146,17 +146,17 @@ object UnsafeRow { | |||
| case struct: TStruct => | |||
| readStruct(region, offset, ttBc) | |||
|
|
|||
| case TVariant => | |||
| val ft = TVariant.fundamentalType.asInstanceOf[TStruct] | |||
| case x: TVariant => | |||
There was a problem hiding this comment.
You use x sometimes, t sometimes. I'd just use x.
| @@ -8,8 +8,38 @@ import is.hail.utils._ | |||
| import org.json4s._ | |||
| import org.json4s.jackson.JsonMethods | |||
|
|
|||
| abstract class GRBase extends Serializable { | |||
| def isValidContig(contig: String): Boolean = ??? | |||
There was a problem hiding this comment.
I think it's cleaner to put ??? implementations on GRVariable and have these abstract with no overrides.
|
Looks good. A few cosmetic comments, I won't push for them, change if you agree. |
* Reference Genome #3 - Added genome reference to TVariant, TInterval, TLocus * fixed tvariant sig match * fixed type unify error * fixed rebase * fixed hailrep for variant * fixed hailrep for locus and interval * fix tests * add GRVariable * Attempt #3 * Changed vSig for GDB * fixup * addressed comments
