add documention for VariantContext.getStart() regarding telomeric events #1369

SHuang-Broad · 2019-05-17T21:02:23Z

Description

The VCF spec allows POS column to take value 0 (or chromosome length + 1), when the event is at a telomere.

Currently the documentation for public int VariantContext.getStart() claims it returns a 1-based value, where it seems that 0 is an invalid value.

This PR intends to clarify that, by adding documentation.

Checklist

(Documentation change, most of the following does not apply)

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

codecov-io · 2019-05-17T21:14:56Z

Codecov Report

Merging #1369 into master will decrease coverage by 0.009%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##             master     #1369       +/-   ##
==============================================
- Coverage     67.85%   67.841%   -0.009%     
+ Complexity     8283      8282        -1     
==============================================
  Files           564       564               
  Lines         33695     33695               
  Branches       5650      5650               
==============================================
- Hits          22862     22859        -3     
- Misses         8653      8655        +2     
- Partials       2180      2181        +1

Impacted Files	Coverage Δ	Complexity Δ
.../htsjdk/variant/variantcontext/VariantContext.java	`77.714% <ø> (ø)`	`246 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/BAMFileReader.java	`67.847% <0%> (-0.817%)`	`51% <0%> (-1%)`

vruano · 2019-05-19T05:44:56Z

src/main/java/htsjdk/variant/variantcontext/VariantContext.java

@@ -1664,7 +1664,15 @@ public String getContig() {

    /**
     * @return 1-based inclusive start position of the Variant


Since you are here, I would reformat the javadoc like this:

/** * Summary sentence. * * explanation of what it does without loosing time in details of particular edge cases. * * * edge case-1 * * * edge case-2 * * * @return the text above should had made clear wha is returned... here you report on the possible range of values very briefly. */

For example in this case something like this: /** * Returns the (start) position of the variant. * * Main blah blah ... 1-based ... blah blah. * * * For telomeres blah blah can be 0 or N+1 blah blah * * @return 0 or greater, never a negative number. */

Here I'm just putting emphasis in the summary sentence (first one finished with .) and the statement in the [at]return tag, the details in the section in between is up to you (that is way I add those "blah" "blah").

That said I think you should be more concise... The programmer don't want to expend to much time to understand what may happen in 0.1% percent of cases. For example I would not include details on what is said or depicted in the spec, just simply refer to it for further reading/details.

SHuang-Broad · 2019-05-19T16:07:39Z

@vruano Thanks for taking a look!
Please take a look at the re-formatted doc.

vruano · 2019-05-19T20:33:11Z

In my view is just too much text; think about the 99.5% of people that don't care about telomeres. but I rather you get some other opinion as perhaps I'm just too pedantic.

About the summary and [at]return annotation. I mean something like this:

/**
 * Returns the position for this variant.
 *
 * @return 0 or greater.
 */

The summary sentence is "what it does" very briefly whareas the [at]return is just give info about the possible range of values without semantics (this goes into the summary and the rest of the javadoc. For example if the return was Object the often is either @return never {@code null} or @return might be {@code null}. Sometimes if the method is know to return this (e.g. StringBuilder append methods) then I also say that @return this builder (in this case it is imply that it's never null.

vruano · 2019-05-19T21:54:25Z

I would add warning people that work to this level with VCF data are supposed to be aware of this.

SHuang-Broad · 2019-05-20T03:42:40Z

I agree with you that the summary was too long.
So I shrunk it further and added a warning to the new note about telomere.

vruano · 2019-05-20T14:05:15Z

src/main/java/htsjdk/variant/variantcontext/VariantContext.java

@@ -1663,8 +1663,11 @@ public String getContig() {
    }

    /**
-     * Returns 1-based inclusive start position of the variant, 0 or greater.
-     * See below for explanation on "0".
+     * Returns 1-based inclusive start position of the variant.


I would drop the "start" since this is in fact controversial, despite that it is the method's name.

Also would people know what "1-based inclusive" mean? perhaps that should be remove from here and or explained in another "
" block... but since this not the only place we use 1-based indexes ... so do you even need to explain it here..... I guess it won't hurt.

I would drop the "start" since this is in fact controversial, despite that it is the method's name.

Why is "start" controversial?

I don't quite understand the rest of the comments

vruano · 2019-05-20T14:10:06Z

src/main/java/htsjdk/variant/variantcontext/VariantContext.java

+     * Returns 1-based inclusive start position of the variant.
+     *
+     * <p>
+     *     INDEL events usually start on the first unaltered reference base before the INDEL.


This is a bit confussing... INDEL event actual start in the bae before the INDEL... not really ... the indel start where it starts... perhaps you mean to say that it is reported one base before.
Also perhaps you should try to be more general here... instead of talking about INDEL alone you could say something like:
"Notice that for some types of variant events the actual start position may not be this value (e.g. deletions are reported on the base before the first base deleted)."

So you are not giving the impression that it may only happen with deletions or indels.

This is directly copied from the old comment.

But I agree that it can be more general.

vruano · 2019-05-20T14:23:37Z

src/main/java/htsjdk/variant/variantcontext/VariantContext.java

     *     Note also that the VCF spec allows 0 and N + 1 for POS field for telomeric event,
     *     where N is the length of the chromosome.
     *     The "0" value returned should be interpreted as telomere, and does not violate the above "1-based" comment.
-     *     It's the responsibility of code generating such variants to make sure {@code start} is populated correctly.


Is unfortunate the the spec talks about "Telomeres" only. It is assuming that the CHROM (thus the referene) only can contain full non-circular chromosomes.... total BS. (e.g. the MT chromosome in humans or nearly every contig in unfinished genomic references)
Yes CHROM as a name wasn't a good choice to start with but we don't need to keep up to that mistake. Notice that API using "contig" instead of "chr" or "chromosome; it is still assuming too much but is closer to the truth..

So I would refrain to make it seems as it can only be telomerees.

What about something like:

"
This property can take on "0" and "N+1" (where N is the last base in the enclosing contig) when this variant record makes references to events that happen before or at the beginning or after or at the end of the enclosing contig."

I hope this PR does not turn into a loath about the spec itself. I can only work within the current spec for this PR.
So, since the latest spec talks about telomere, I'm happy to use a single word "telomere" to avoid such long sentence.

It won't, I leave it up to you at this point.

vruano · 2019-05-20T14:31:06Z

Sorry perhaps I went to far giving examples ... can borrow mine but you can/should use your own words.

pshapiro4broad · 2019-06-24T18:39:24Z

src/main/java/htsjdk/variant/variantcontext/VariantContext.java

+     *
+     * <p>
+     *     INDEL events usually start on the first unaltered reference base before the INDEL.
+     * </p>


 markup is unnecessary as the  tag closes the previous open paragraph tag.

lbergelson

I think the comments are valid but I don't think they have to be addressed as part of this change, this is just mentioning the existence of telomere and N+1 positions as possibilities.

add documention for VariantContext.getStart() regarding telomeric events

48a11dd

vruano suggested changes May 19, 2019

View reviewed changes

SHuang-Broad added 2 commits May 19, 2019 12:06

reformating doc following comments

f528993

oops

5f3229b

2nd round

b2961e5

vruano reviewed May 20, 2019

View reviewed changes

pshapiro4broad reviewed Jun 24, 2019

View reviewed changes

lbergelson approved these changes Jul 1, 2019

View reviewed changes

lbergelson merged commit 9aa81ed into samtools:master Jul 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add documention for VariantContext.getStart() regarding telomeric events #1369

add documention for VariantContext.getStart() regarding telomeric events #1369

SHuang-Broad commented May 17, 2019 •

edited by vruano

Loading

codecov-io commented May 17, 2019 •

edited

Loading

vruano May 19, 2019

vruano May 19, 2019

vruano May 19, 2019

SHuang-Broad commented May 19, 2019

vruano commented May 19, 2019 •

edited

Loading

vruano commented May 19, 2019

SHuang-Broad commented May 20, 2019

vruano May 20, 2019

vruano May 20, 2019

SHuang-Broad May 23, 2019

vruano May 20, 2019

SHuang-Broad May 23, 2019

vruano May 20, 2019

SHuang-Broad May 23, 2019

vruano May 24, 2019

vruano commented May 20, 2019

pshapiro4broad Jun 24, 2019

lbergelson left a comment

		@@ -1664,7 +1664,15 @@ public String getContig() {

		/**
		* @return 1-based inclusive start position of the Variant

add documention for VariantContext.getStart() regarding telomeric events #1369

add documention for VariantContext.getStart() regarding telomeric events #1369

Conversation

SHuang-Broad commented May 17, 2019 • edited by vruano Loading

Description

Checklist

codecov-io commented May 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SHuang-Broad commented May 19, 2019

vruano commented May 19, 2019 • edited Loading

vruano commented May 19, 2019

SHuang-Broad commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vruano commented May 20, 2019

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

SHuang-Broad commented May 17, 2019 •

edited by vruano

Loading

codecov-io commented May 17, 2019 •

edited

Loading

vruano commented May 19, 2019 •

edited

Loading