Hello Cassie et al,

Sorry, only getting to look at this carefully now.  Lots of good things, but I have various comments.  Some are specific, some more general.

- You need a set of reference sequences for the coordinate system to mean something.  In my new world proposed in reads.avdl (#66)
this would be a GAReferenceSequenceSet and your contig would be a referenceSequenceName, but even before #66 is adopted
you somewhere need an array<GAReferenceSequence>.  Perhaps in the DataSet?  Can we require DataSet to just have one reference
sequence set?

- I suggest the position should be 0-based inside this GA world.  Fine to convert to 1-based when we export to VCF, but I think it is cleaner
to use 0-based coordinates and semi-open intervals [start,end) inside GA.  So [0,length) is the first length bases of the sequence.  This is
what Benedict Paten and we have done in record GAThread in referenceStructure.avdl.  If we do this we should also do it in GALinearAlignment,
which may mean changing working prototypes....  Hmm - it it too late for this?

- reference_bases had better be non-null.  The semantics are literal replacement of reference_bases by alternate_bases, so we have to
know at least how long reference_bases is, and if something is inserted then whether that is before or after.  Even with very complex
structural variations VCF requires a literal reference_bases.

- Ahead of the array of calls, VCF includes in FORMAT the properties that are present for each call.  Do you think you might want something like

	array<string> callKeys = [];    // keys used in info within calls for this variant

- It would be good to set up structures to type the keys used in the info fields inGAVariant and GACall.  This is information given in the VCF header.

- I gather that your GACallSet corresponds to a sample.  Shouldn't it somehow be equated to the same sample who is given for sequencing
libraries etc.?  Why did you not want to use the word Sample?

- Why have the callset_name as well as callset_id in the call object?

My biggest problem is with tying a Variant to a particular collection of callsets/samples, and indeed to this all being tied to a DataSet.  
I am not clear what a DataSet is.  Is this the same object that readgroups belong to?  If we want to consider the genotypes for same variant 
on a different set of samples, for example a subset, it looks like we need a new Variant object.  I guess some of the information might
change (e.g. NA, NS info numbers) but other information remains the same.  A lot of these issues arise also with VCF from the file nature
of VCF, but it is common to make new VCFs from old ones by slicing, projecting, merging, etc.

Richard

On 23 May 2014, at 19:25, cassiedoll <notifications@github.com> wrote:

> In src/main/resources/avro/variants.avdl:
> 
> > +  // If a variant had a referenceBases field of "T", an alternateBases
> > +  // value of ["A", "C"], and the genotype was [2, 1], that would mean the call
> > +  // represented the heterozygous value "CA" for this variant. If the genotype
> > +  // was instead [0, 1] the represented value would be "TA". Ordering of the
> > +  // genotype values is important if the phaseset field is present.
> > +  array<int> genotype = [];
> > +
> > +  // If this field is present, this variant call's genotype ordering implies
> > +  // the phase of the bases and is consistent with any other variant calls on
> > +  // the same contig which have the same phaseset value.
> > +  union { null, string } phaseset = null;
> > +
> > +  // The genotype likelihoods for this variant call. Each array entry
> > +  // represents how likely a specific genotype is for this call. The value
> > +  // ordering is defined by the GL tag in the VCF spec.
> > +  array<double> genotype_likelihood = [];
> Oh and for different variant types - it depends on how different we think the fields of each type need to be. If they are 99% the same, then we're all good. If they are only 50% the same, well, then we can think about having different types of variants with different fields. (there are many ways to support that in an API if we think its the right choice)
> 
> —
> Reply to this email directly or view it on GitHub.
> 




-- 
 The Wellcome Trust Sanger Institute is operated by Genome Research 
 Limited, a charity registered in England with number 1021457 and a 
 company registered in England with number 2742969, whose registered 
 office is 215 Euston Road, London, NW1 2BE.