Move bad coordinates check #911

ronlevine · 2017-06-24T15:31:16Z

Description

Follow up to #908.
Move the bad coordinates check to the root method, public Iterator query(final String reg). If this public method is used by an application, the bad coordinates will return an EOF_ITERATOR;

Checklist

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)

ronlevine · 2017-06-24T15:32:03Z

@yfarjoun Please review.

codecov-io · 2017-06-24T15:43:00Z

Codecov Report

Merging #911 into master will increase coverage by 0.007%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master      #911       +/-   ##
===============================================
+ Coverage     65.084%   65.091%   +0.007%     
- Complexity      7265      7268        +3     
===============================================
  Files            528       528               
  Lines          31948     31946        -2     
  Branches        5455      5453        -2     
===============================================
+ Hits           20793     20794        +1     
  Misses          9011      9011               
+ Partials        2144      2141        -3

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/htsjdk/tribble/readers/TabixReader.java	`68.293% <100%> (+0.148%)`	`61 <16> (+1)`	⬆️
...dk/samtools/seekablestream/SeekableHTTPStream.java	`56.061% <0%> (+1.515%)`	`10% <0%> (+1%)`	⬆️
...a/htsjdk/samtools/cram/encoding/rans/Decoding.java	`85.714% <0%> (+3.571%)`	`12% <0%> (+1%)`	⬆️

yfarjoun

minor comment about spaces. also, could you put an issue that parseReg doesn't work for sequences that have colons or dashes in their names?

src/test/java/htsjdk/tribble/readers/TabixReaderTest.java

yfarjoun · 2017-10-02T11:33:12Z

@ronlevine are you still available to process this?

if not @lbergelson what's the procedure?

yfarjoun · 2017-10-10T15:45:46Z

@cmnbroad can you take over this?

cmnbroad · 2017-10-10T19:07:00Z

@yfarjoun Sure.

cmnbroad · 2017-10-24T15:19:44Z

@yfarjoun I don't see anything in this PR that addresses the second issue raised in the ticket. A couple of questions:

Do you think we should we retain the current behavior where querying a non-existent sequence name silently returns an empty iterator ? It seems to me it should throw. Same for start index <= 0. Such queries seem incoherent, but the caller has no way to notify the user. It would be a behavior change though.
It looks like the parseReg method is converting com 1-based to 0-based, and subtracts 1 from the start, but not the end. Unless its intentionally converting from a fully closed interval to half-closed, half-open, that seems wrong. Do you know if thats intentional ? I'd like to add some comments/javadoc and boundary condition tests, but I'm not sure what the intentions are.

src/main/java/htsjdk/tribble/readers/TabixReader.java

droazen · 2017-11-14T19:30:01Z

src/test/java/htsjdk/tribble/readers/TabixReaderTest.java

+        iter=tabixReader.query("1",Integer.MAX_VALUE,-1);
+        Assert.assertNotNull(iter);
+        Assert.assertNull(iter.next());
+


Can we add a test case in which end == 0?

Also add test cases for the other overloads of query(), since they have difference interval conventions (the String version is 1-based closed, the 3-arg version is 0-based closed-open!)

droazen · 2017-11-14T19:34:49Z

Review complete, back to @cmnbroad

yfarjoun · 2018-05-01T18:14:26Z

ping....

fleharty · 2018-07-02T18:18:41Z

@yfarjoun I'm taking this over, but don't have the ability to assign since I don't have commit privileges.

yfarjoun · 2018-10-29T18:23:16Z

assigned to you @fleharty

fleharty · 2018-12-03T18:47:56Z

@droazen Back to you.

yfarjoun · 2019-01-24T20:54:20Z

@droazen ? @cmnbroad ? who's court is this in now?

fleharty · 2019-01-24T21:02:39Z

@yfarjoun my court

codecov-io · 2019-02-04T19:17:30Z

Codecov Report

Merging #911 into master will increase coverage by 0.016%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##             master      #911       +/-   ##
==============================================
+ Coverage      69.5%   69.517%   +0.016%     
- Complexity     8399      8403        +4     
==============================================
  Files           558       558               
  Lines         33371     33369        -2     
  Branches       5610      5608        -2     
==============================================
+ Hits          23193     23197        +4     
+ Misses         7904      7901        -3     
+ Partials       2274      2271        -3

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/htsjdk/tribble/readers/TabixReader.java	`73.707% <100%> (+0.63%)`	`66 <17> (+3)`	⬆️
...va/htsjdk/samtools/util/AsyncBufferedIterator.java	`69.231% <0%> (+1.099%)`	`17% <0%> (ø)`	⬇️
...samtools/util/AsyncBlockCompressedInputStream.java	`76% <0%> (+4%)`	`13% <0%> (+1%)`	⬆️

yfarjoun · 2019-02-05T14:08:13Z

looks good now.

ronlevine self-assigned this Jun 24, 2017

ronlevine requested a review from yfarjoun June 24, 2017 15:31

yfarjoun approved these changes Oct 2, 2017

View reviewed changes

src/test/java/htsjdk/tribble/readers/TabixReaderTest.java Outdated Show resolved Hide resolved

cmnbroad assigned cmnbroad and unassigned ronlevine Oct 10, 2017

cmnbroad mentioned this pull request Oct 23, 2017

Tabix doesn't handle queries using sequence names containing ':' or '-' #1017

Open

droazen reviewed Nov 14, 2017

View reviewed changes

src/main/java/htsjdk/tribble/readers/TabixReader.java Outdated Show resolved Hide resolved

droazen reviewed Nov 14, 2017

View reviewed changes

src/main/java/htsjdk/tribble/readers/TabixReader.java Outdated Show resolved Hide resolved

droazen reviewed Nov 14, 2017

View reviewed changes

yfarjoun unassigned cmnbroad Jun 18, 2018

yfarjoun assigned fleharty Oct 29, 2018

fleharty force-pushed the rhl_tabix_read_bad_coord_908 branch from 0244ade to c50da95 Compare January 14, 2019 20:58

ronlevine and others added 2 commits February 4, 2019 14:15

Move bad coordinates check

bb42cbb

Responding to David R's comments

0e4e15a

fleharty force-pushed the rhl_tabix_read_bad_coord_908 branch from 820362a to 0e4e15a Compare February 4, 2019 19:17

yfarjoun approved these changes Feb 5, 2019

View reviewed changes

yfarjoun merged commit 3ae552f into master Feb 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move bad coordinates check #911

Move bad coordinates check #911

ronlevine commented Jun 24, 2017 •

edited

Loading

ronlevine commented Jun 24, 2017

codecov-io commented Jun 24, 2017 •

edited

Loading

yfarjoun left a comment

yfarjoun commented Oct 2, 2017

yfarjoun commented Oct 10, 2017

cmnbroad commented Oct 10, 2017

cmnbroad commented Oct 24, 2017

droazen Nov 14, 2017

droazen Nov 14, 2017

droazen commented Nov 14, 2017

yfarjoun commented May 1, 2018

fleharty commented Jul 2, 2018

yfarjoun commented Oct 29, 2018

fleharty commented Dec 3, 2018

yfarjoun commented Jan 24, 2019

fleharty commented Jan 24, 2019

codecov-io commented Feb 4, 2019

yfarjoun commented Feb 5, 2019

Move bad coordinates check #911

Move bad coordinates check #911

Conversation

ronlevine commented Jun 24, 2017 • edited Loading

Description

Checklist

ronlevine commented Jun 24, 2017

codecov-io commented Jun 24, 2017 • edited Loading

Codecov Report

yfarjoun left a comment

Choose a reason for hiding this comment

yfarjoun commented Oct 2, 2017

yfarjoun commented Oct 10, 2017

cmnbroad commented Oct 10, 2017

cmnbroad commented Oct 24, 2017

droazen Nov 14, 2017

Choose a reason for hiding this comment

droazen Nov 14, 2017

Choose a reason for hiding this comment

droazen commented Nov 14, 2017

yfarjoun commented May 1, 2018

fleharty commented Jul 2, 2018

yfarjoun commented Oct 29, 2018

fleharty commented Dec 3, 2018

yfarjoun commented Jan 24, 2019

fleharty commented Jan 24, 2019

codecov-io commented Feb 4, 2019

Codecov Report

yfarjoun commented Feb 5, 2019

ronlevine commented Jun 24, 2017 •

edited

Loading

codecov-io commented Jun 24, 2017 •

edited

Loading