Suggesting <NON_REF> as the unspecified alt allele #380

lbergelson · 2019-02-06T20:31:57Z

Adding <NON_REF> to the spec and recommending its use over the <*> allele which is easily confused with *.
closes <*> allele vs <NON_REF> #352

lbergelson · 2019-02-06T21:00:42Z

I don't know why circle-ci ran on this and failed... Strange.

cyenyxe

A general question: I have noticed that GATK outputs VCF v4.2 but this change is only applied to v4.3, because that is when the gVCF section was added. How does this affect you @lbergelson ? Would GATK migrate to v4.3 if this was added?

cyenyxe · 2019-02-06T21:44:36Z

VCFv4.3.tex

@@ -1347,19 +1347,20 @@ \subsection{Representing unspecified alleles and REF-only blocks (gVCF)}
 In order to report sequencing data evidence for both variant and non-variant positions in the genome, the VCF specification allows to represent blocks of reference-only calls in a single record using the END INFO tag, an idea originally introduced by the gVCF file format\footnote{\url{https://help.basespace.illumina.com/articles/descriptive/gvcf-files/}}.
 The convention adopted here is to represent reference evidence as likelihoods against an unknown alternate allele.
 Think of this as the likelihood for reference as compared to any other possible alternate allele (both SNP, indel, or otherwise).
-A symbolic alternate allele $<$*$>$ is used to represent this unspecified alternate allele.
+Either of the two symbolic alleles $<$*$>$ or $<$NON\_REF$>$ may be used to represent this unspecified alternate allele.


Given that we are recommending the usage of NON_REF, maybe it should be listed first?

Good point, I've switched them.

lbergelson · 2019-02-06T21:50:34Z

@cyenyxe That's a good question about 4.3 vs 4.2. Gatk doesn't output 4.3 due to technical problems in htsjdk. We're planning on fixing it but it hasn't happened yet. This PR isn't a deciding factor either way.

I think we could probably backport port the gvcf blurb to the 4.2 spec since it's really more of a clarification about a specific use of VCF than a change in the spec and clearly gvcfs have existed before 4.2.

If you think we should include this note in the 4.2 spec I can move that back as well.

cyenyxe · 2019-02-07T08:09:11Z

I could be fine with backporting the gVCF part, as it doesn't break compatibility and would make GATK VCFs (not a small number) fully compliant against the spec. What do you think @pd3?

pd3 · 2019-02-07T08:44:58Z

@lh3 came up with the idea of the space-holder for an unseen allele in samtools/bcftools and when GATK started using it as well, we agreed on the symbolic star allele <*>. Samtools/bcftools swiftly changed from its original X and <X>. Atlhough GATK developers were active in drafting the specification and agreed on <*> years ago, somehow it still remains an issue. I don't understand why and in my opinion allowing <NON_REF> now would set a very bad precedent.

cyenyxe · 2019-02-07T08:51:53Z

The problem with <*> is that it is an extremely confusing notation (see comments from non-GATK developers in #352). We can always create a brief poll to understand how people are using them, and which one would they prefer in the long term.

But the truth is that there are many VCF files out there already using NON_REF.

pd3 · 2019-02-07T08:58:05Z

I agree the notation is not well described in the specification, but I disagree it is extremely confusing.

The correct way forward for GATK is to switch to producing <*> and silently continue supporting also <NON_REF> for backward compatibility, rather than enforce all other tools to start supporting both. It is programmatically very easy to do and if both alleles should become a standard as in this proposal, it would have to support both anyway!

tfenne · 2019-02-07T12:45:29Z

A few thoughts:

@pd3 I have personally interacted with enough folks confused by * vs <*> that I find it hard to agree that it's not confusing.
I'm not sure the historical argument holds water. As @lbergelson points out in <*> allele vs <NON_REF> #352 at least as far as he could see GATK first started using <NON_REF> in 2013, before any of the other tools mentioned used an allele for this purpose.
To be fair though, there was a lot of turnover on the GATK team IIRC in the 2013-2015 timeframe, so it's possible that someone agreed to the change to <*> and that agreement got lost in the churn. I certainly don't think anyone was acting in bad faith by not changing to <*> at the time. Is there any chance that discussion happened in email/git issues vs. phone calls?
As a user of VCF I feel like VCF could adopt the PERL slogan of "there's more than one way to do it" - so many things in VCF are encoded differently by different callers, having two ways of doing it seems perfectly normal.

Here's an idea: how about instead of extending the spec to officially recognize <*> and <NON_REF>, having a way to use the ##ALT header line to explicitly declare in a VCF what if any allele is used for this purpose by adding a NONREF type? And then perhaps stating that if not present <*> and <NON_REF> are used by existing tools and may be expected?

pd3 · 2019-02-07T13:40:03Z

@tfenne, on my hard drive lives the file samtools/bam2bcf.c with a timestamp from Dec 20 2011 with the following line

kputc(bc->unseen == i? 'X' : "ACGT"[bc->a[i]], &s);

and very likely it is not the first version of it.

My points are following:

The current GATK's <NON_REF> allele does not break the VCF specification, therefore you can continue using it.
We tried to standardize this years ago and the consensus was that the symbolic star allele<*> would become reserved in the specification, not <X> and not <NON_REF>.
If people find <*> vs * confusing, this is still only a problem of documenting it better.

Although I personally would have preferred to continue using the <X> notation, we switched to <*> for the sake of supporting the standard. I find it disconcerting that now, years after, the issue is still alive.

As for your suggestion, the use of the symbolic non-ref allele is very specific and I am not convinced that it is worthwhile to complicate further the ##ALT meta-information lines.

jkbonfield · 2019-02-07T17:25:41Z

My take on this, as an independent non-VCF person, is that if things don't break the specification but are in common usage then it doesn't harm to document them.

However we need to be clear to distinguish normative specification text and explanatory / clarificatory text.

If the scale of the problem is simply too vast (eg the TLEN problem of 5' to 3' vs left-most to right-most is dominated by code doing the wrong thing, because the spec changed from one thing to another and hardly anyone updated their code), then documenting both is a pragmatic alternative but not an ideal scenario. I'd be wary of blessing one over the other though - if you go down the road of permitting both then you are just accepting both will exist forever.

If you want to get only method and feel it is achievable, then don't label both as permitted - just document the unsatisfactory situation and emphasise the recommended practice is the current behaviour.

lh3 · 2019-02-27T21:52:59Z

Although I personally would have preferred to continue using the <X> notation

I prefer <X>, too, but I am fine with <*>.

on my hard drive lives the file samtools/bam2bcf.c with a timestamp from Dec 20 2011 with the following line

Bcftools was separated from samtools in samtools v0.1.9, released on 2010-10-27. Bcftools had been using X as the unspecified allele from the very beginning. As I vaguely remember, the VCF spec didn't have the concept of symbolic allele at the time. I could be wrong, though.

Personally, I am ok to add <NON_REF> as an alternative to <*> in principle, but I think <*> should be the recommended encoding in the spec. That was the decision. Also, if we add <NON_REF>, htsjdk/GATK should also support both. @pd3: would supporting <NON_REF> cause much trouble to htslib?

Here's an idea: how about instead of extending the spec to officially recognize <*> and <NON_REF>, having a way to use the ##ALT header line to explicitly declare in a VCF what if any allele is used for this purpose by adding a NONREF type? And then perhaps stating that if not present <*> and <NON_REF> are used by existing tools and may be expected?

If we add <NON_REF>, I much prefer to make them always equivalent. The VCF spec is complex enough. Having a new header line seems too complicated.

* Adding <NON_REF> to the spec and recommending its use over the <*> allele which is easily confused with *. * closes samtools#352

hts-specs-bot · 2019-05-30T15:22:51Z

Changed PDFs as of b3f5e40: VCFv4.3 (diff).

hts-specs-bot · 2019-06-24T10:23:25Z

Changed PDFs as of e90af6e: VCFv4.3 (diff).

cyenyxe · 2019-06-24T10:24:13Z

After several offline discussions, I have updated this to acknowledge the existence of <NON_REF>, but keep <*> as the preferred allele.

lbergelson · 2019-07-24T19:50:28Z

VCFv4.3.tex

@@ -1362,7 +1362,7 @@ \subsection{Representing unspecified alleles and REF-only blocks (gVCF)}
 In order to report sequencing data evidence for both variant and non-variant positions in the genome, the VCF specification allows to represent blocks of reference-only calls in a single record using the END INFO tag, an idea originally introduced by the gVCF file format\footnote{\url{https://help.basespace.illumina.com/articles/descriptive/gvcf-files/}}.
 The convention adopted here is to represent reference evidence as likelihoods against an unknown alternate allele.
 Think of this as the likelihood for reference as compared to any other possible alternate allele (both SNP, indel, or otherwise).
-A symbolic alternate allele $<$*$>$ is used to represent this unspecified alternate allele.
+A symbolic alternate allele $<$*$>$ is used to represent this unspecified alternate allele. This is preferred over the symbolic allele $<$NON\_REF$>$, used by the Genome Analysis Toolkit (GATK).


@cyenyxe This seems like the right solution, the community has definitely overruled my initial suggestion to promote <NON_REF> over <*>

Suggested change

A symbolic alternate allele $<$*$>$ is used to represent this unspecified alternate allele. This is preferred over the symbolic allele $<$NON\_REF$>$, used by the Genome Analysis Toolkit (GATK).

A symbolic allele $<$*$>$ is used to represent this unspecified alternate allele.

This is preferred over the equivalent allele $<$NON\_REF$>$.

I think we have a redundant alternate in there, and we should split multiple sentences on multiple lines to match the formatting.

I think it's good to specify that they have the same meaning, and I don't know if it's necessary to include the GATK by name here since we don't usually refer to specific implementations.

It might also be useful to highlight in some way the term unspecified alternate allele if that's how we intend it to be pronounced, since my major complaint with <*> is that it's pronunciation causes confusion.

hts-specs-bot · 2019-08-22T12:19:10Z

Changed PDFs as of 19df07f: VCFv4.3 (diff).

CRAMv3 PRs #401 and #412. VCFv4.3 PRs #380 (<NON_REF>), #409 (infinity/NaN), and #436 (INFO/END). All: Minor typo and whitespace formatting fixes.

* Update CRAM spec section on substitution matrix and codes. * Respond to review comments. * CRAM Slice and Container ref seq IDs must match (samtools#401) * Code review part 2. * Fix minor typo in the predecessors of BCF2 (samtools#427) * layed -> laid * Update MAINTAINERS.md (samtools#432) Proposal to add Rasko Leinonen as refget maintainer. * add jmmut to MAINTAINERS.md and move Cristina to "Past Members" * change order of maintainers * Clarify that INFO/END is used to form a CHROM:POS-END region (PR samtools#436) (samtools#436) INFO/END (when present) provides the size of the interval that the variant is located in, along with the CHROM and POS fields. This is also used when indexing VCF/BCF files, as can be gleaned from §6.3.1's description of BCF's rlen field. The implications of INFO/END have not previously been clear. In the absence of clear documentation, some SV tools have been using INFO/END fields for their own semi-related purposes (using INFO/CHR2:INFO/END as the other side's position in an interchromosomal rearrangement), leading to broken .csi indexes and region queries that don't work. Fixes samtools#425. * Allow C and Java native text spellings of NaN and infinities (samtools#409) C's printf doesn't output mixed case, while Java's Double.valueOf and Double.toString parse/output only `Infinity`, not `Inf`. Rather than requiring special-case code for both input and output in both languages, relax the VCF specification to allow NAN/INF/INFINITY case-insensitively. (Add "IEEE-754" to be specific and to improve the line breaks.) * Adding a note about <NON_REF> (samtools#380) * Update PDFs (CRAM and VCF additions; others cosmetic) CRAMv3 PRs samtools#401 and samtools#412. VCFv4.3 PRs samtools#380 (<NON_REF>), samtools#409 (infinity/NaN), and samtools#436 (INFO/END). All: Minor typo and whitespace formatting fixes. * Add htsget 1.2.0 OpenAPI v3.0.2 spec (PR samtools#385) Includes barebones authorizationCode Oauth2 flow, which should aid/inform code generation. Uses int64 with minimum 0, unsure if that is really an uint64 though. * Codify the existing policy of generally squashing PRs (PR samtools#444) * Permit AP_Delta in multi-ref slices. This means AP_delta can become negative. I have validated this decodes fine in both htsjdk and htslib. This is because AP is ITF8 and hence signed, like all other integers, so it would need explicit code to forbid this (which obviously isn't in the implementations). Hence the limitation is primarily one of an over-zealous specification. The impact of this is for position-sorted multi-ref slices AP can legally be stored efficiently. Also clarified fields in the container compression header when in multi-ref mode. Fixes samtools#431

lbergelson requested review from cyenyxe and pd3 February 6, 2019 20:59

lbergelson added the vcf label Feb 6, 2019

cyenyxe reviewed Feb 6, 2019

View reviewed changes

lbergelson added 3 commits May 30, 2019 11:05

Suggesting <NON_REF> as the unspecified alt allele

5e93817

* Adding <NON_REF> to the spec and recommending its use over the <*> allele which is easily confused with *. * closes samtools#352

rewording

b61bd85

changing order

b3f5e40

lbergelson force-pushed the lb_non_ref branch from 6f532db to b3f5e40 Compare May 30, 2019 15:21

Cristina Yenyxe Gonzalez Garcia added 2 commits June 24, 2019 11:19

Asterisk symbolic allele preferred over NON_REF

bf47235

Restored asterisk symbolic allele in examples

e90af6e

lbergelson commented Jul 24, 2019

View reviewed changes

Made the notation and name link more explicit

19df07f

lbergelson merged commit fb7120f into samtools:master Aug 22, 2019

lbergelson deleted the lb_non_ref branch August 22, 2019 15:14

jmarshall added a commit that referenced this pull request Aug 22, 2019

Update PDFs (CRAM and VCF additions; others cosmetic)

c6f6d93

CRAMv3 PRs #401 and #412. VCFv4.3 PRs #380 (<NON_REF>), #409 (infinity/NaN), and #436 (INFO/END). All: Minor typo and whitespace formatting fixes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggesting <NON_REF> as the unspecified alt allele #380

Suggesting <NON_REF> as the unspecified alt allele #380

lbergelson commented Feb 6, 2019

lbergelson commented Feb 6, 2019

cyenyxe left a comment •

edited

Loading

cyenyxe Feb 6, 2019

lbergelson Feb 6, 2019

lbergelson commented Feb 6, 2019

cyenyxe commented Feb 7, 2019

pd3 commented Feb 7, 2019

cyenyxe commented Feb 7, 2019

pd3 commented Feb 7, 2019

tfenne commented Feb 7, 2019

pd3 commented Feb 7, 2019

jkbonfield commented Feb 7, 2019 •

edited

Loading

lh3 commented Feb 27, 2019 •

edited

Loading

hts-specs-bot commented May 30, 2019

hts-specs-bot commented Jun 24, 2019

cyenyxe commented Jun 24, 2019

lbergelson Jul 24, 2019

hts-specs-bot commented Aug 22, 2019

	A symbolic alternate allele $<$*$>$ is used to represent this unspecified alternate allele. This is preferred over the symbolic allele $<$NON\_REF$>$, used by the Genome Analysis Toolkit (GATK).
	A symbolic allele $<$*$>$ is used to represent this unspecified alternate allele.
	This is preferred over the equivalent allele $<$NON\_REF$>$.

Suggesting <NON_REF> as the unspecified alt allele #380

Suggesting <NON_REF> as the unspecified alt allele #380

Conversation

lbergelson commented Feb 6, 2019

lbergelson commented Feb 6, 2019

cyenyxe left a comment • edited Loading

Choose a reason for hiding this comment

cyenyxe Feb 6, 2019

Choose a reason for hiding this comment

lbergelson Feb 6, 2019

Choose a reason for hiding this comment

lbergelson commented Feb 6, 2019

cyenyxe commented Feb 7, 2019

pd3 commented Feb 7, 2019

cyenyxe commented Feb 7, 2019

pd3 commented Feb 7, 2019

tfenne commented Feb 7, 2019

pd3 commented Feb 7, 2019

jkbonfield commented Feb 7, 2019 • edited Loading

lh3 commented Feb 27, 2019 • edited Loading

hts-specs-bot commented May 30, 2019

hts-specs-bot commented Jun 24, 2019

cyenyxe commented Jun 24, 2019

lbergelson Jul 24, 2019

Choose a reason for hiding this comment

hts-specs-bot commented Aug 22, 2019

cyenyxe left a comment •

edited

Loading

jkbonfield commented Feb 7, 2019 •

edited

Loading

lh3 commented Feb 27, 2019 •

edited

Loading