Add documentation and args check for SVInterval

[SHuang-Broad]

@tedsharpe
not sure if the args checking would significantly affect performance.

[codecov-io]

Codecov Report

Merging #5157 into master will increase coverage by 0.029%.
The diff coverage is 53.125%.

@@               Coverage Diff               @@
##              master     #5157       +/-   ##
===============================================
+ Coverage     86.739%   86.767%   +0.029%     
- Complexity     29467     29948      +481     
===============================================
  Files           1818      1818               
  Lines         136389    137607     +1218     
  Branches       15121     15430      +309     
===============================================
+ Hits          118302    119398     +1096     
- Misses         12647     12746       +99     
- Partials        5440      5463       +23

Impacted Files	Coverage Δ	Complexity Δ
...tools/spark/sv/evidence/XGBoostEvidenceFilter.java	89.13% <100%> (+0.18%)	45 <2> (ø)	⬇️
...te/hellbender/tools/spark/sv/utils/SVInterval.java	73.846% <42.857%> (-13.654%)	36 <3> (ø)
...spark/sv/evidence/FindBreakpointEvidenceSpark.java	69.057% <57.143%> (-0.078%)	63 <0> (ø)
...walkers/haplotypecaller/HaplotypeCallerEngine.java	78.754% <0%> (-0.752%)	103% <0%> (+27%)
...nder/tools/walkers/genotyper/GenotypingEngine.java	68.955% <0%> (-0.466%)	111% <0%> (+42%)
...walkers/genotyper/GenotypingGivenAllelesUtils.java	75% <0%> (ø)	7% <0%> (+2%)	⬆️
...ithwaterman/SmithWatermanIntelAlignerUnitTest.java	60% <0%> (ø)	2% <0%> (ø)	⬇️
...utils/variant/GATKVariantContextUtilsUnitTest.java	86.667% <0%> (+0.054%)	283% <0%> (+140%)	⬆️
...aller/HaplotypeCallerGenotypingEngineUnitTest.java	89.13% <0%> (+0.398%)	29% <0%> (ø)	⬇️
...ls/genomicsdb/GenomicsDBImportIntegrationTest.java	91.336% <0%> (+1.953%)	103% <0%> (+30%)	⬆️
... and 7 more

[tedsharpe]

I don't think this is a good idea.

1. The code is completely indifferent to the origin (1-based or 0-based): all methods work correctly with any consistent definition. Currently, the clients can decide what convention to use.

2. Even negative values for start are conceivably meaningful and do not cause any of the methods to return incorrect results.

3. I have code that uses negative values of the contig to encode "opposite strand". (It's not currently checked in, but it's very convenient.)

I'd really like to keep this class simple and wide open. Otherwise, I think we'll be fighting lots of random exceptions that don't really signal any error condition.

[SHuang-Broad]

Then what about the two methods toSimpleInterval and toBedString?
They are assuming 1-based system, I think.

EDIT
I noticed these two were added after the class were initially implemented.
And I think the assumptions added do signal the confusion this can cause,
so IMHO we should improve the documentation a bit.

[SHuang-Broad]

@tedsharpe I've removed the args checking after our discussion, and added more documentation.
And two mentioned methods are removed now.

[tedsharpe]

Here's an idea. It may be too much work, for now, and I'm fine with this code as it stands, but perhaps others are not. Anyway, the idea is this:

The constructor gets one extra argument: A lambda that serves as a validation function of the form

void validate( int contig, int start, int end );

It either throws an exception or returns silently.
Every client of SVInterval thus documents its intended conventions, and makes certain that those conventions are being adhered to.

[SHuang-Broad]

@tedsharpe not sure if this is what you meant.
Also, I think the "ultimate" solution might be marking SVInterval not final, but allows it to be extended, and child classes should name themselves with convention baked in.
What do you think?

[tedsharpe]

I like this. Not sure the "ultimate" solution is necessary, but it's certainly reasonable.

[SHuang-Broad]

Going to risk it and take the "I like this" as approval 😏