progress logger logs read name too #1180

helgridly · 2018-09-18T21:52:13Z

Description

Running Picard SortSam to sort with queryname gave me output during that didn't tell me much about how done I was.

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)

codecov-io · 2018-09-18T22:10:08Z

Codecov Report

Merging #1180 into master will decrease coverage by <.001%.
The diff coverage is 62.5%.

@@               Coverage Diff               @@
##              master     #1180       +/-   ##
===============================================
- Coverage     68.398%   68.398%   -<.001%     
- Complexity      8013      8015        +2     
===============================================
  Files            541       541               
  Lines          32688     32694        +6     
  Branches        5528      5529        +1     
===============================================
+ Hits           22358     22362        +4     
- Misses          8125      8128        +3     
+ Partials        2205      2204        -1

Impacted Files	Coverage Δ	Complexity Δ
...a/htsjdk/samtools/util/AbstractProgressLogger.java	`41.379% <62.5%> (+0.995%)`	`7 <1> (+1)`	⬆️
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
...va/htsjdk/samtools/sra/SRAIndexedSequenceFile.java	`64.865% <0%> (+2.703%)`	`7% <0%> (+1%)`	⬆️
...sjdk/samtools/util/Md5CalculatingOutputStream.java	`74.359% <0%> (+7.692%)`	`8% <0%> (+1%)`	⬆️

codecov-io · 2018-09-18T22:11:21Z

Codecov Report

Merging #1180 into master will decrease coverage by <.001%.
The diff coverage is 62.5%.

@@               Coverage Diff               @@
##              master     #1180       +/-   ##
===============================================
- Coverage     68.398%   68.398%   -<.001%     
- Complexity      8013      8015        +2     
===============================================
  Files            541       541               
  Lines          32688     32694        +6     
  Branches        5528      5529        +1     
===============================================
+ Hits           22358     22362        +4     
- Misses          8125      8128        +3     
+ Partials        2205      2204        -1

Impacted Files	Coverage Δ	Complexity Δ
...a/htsjdk/samtools/util/AbstractProgressLogger.java	`41.379% <62.5%> (+0.995%)`	`7 <1> (+1)`	⬆️
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
...va/htsjdk/samtools/sra/SRAIndexedSequenceFile.java	`64.865% <0%> (+2.703%)`	`7% <0%> (+1%)`	⬆️
...sjdk/samtools/util/Md5CalculatingOutputStream.java	`74.359% <0%> (+7.692%)`	`8% <0%> (+1%)`	⬆️

codecov-io · 2018-09-18T22:13:29Z

Codecov Report

Merging #1180 into master will decrease coverage by <.001%.
The diff coverage is 62.5%.

@@               Coverage Diff               @@
##              master     #1180       +/-   ##
===============================================
- Coverage     68.398%   68.398%   -<.001%     
- Complexity      8013      8015        +2     
===============================================
  Files            541       541               
  Lines          32688     32694        +6     
  Branches        5528      5529        +1     
===============================================
+ Hits           22358     22362        +4     
- Misses          8125      8128        +3     
+ Partials        2205      2204        -1

Impacted Files	Coverage Δ	Complexity Δ
...a/htsjdk/samtools/util/AbstractProgressLogger.java	`41.379% <62.5%> (+0.995%)`	`7 <1> (+1)`	⬆️
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
...va/htsjdk/samtools/sra/SRAIndexedSequenceFile.java	`64.865% <0%> (+2.703%)`	`7% <0%> (+1%)`	⬆️
...sjdk/samtools/util/Md5CalculatingOutputStream.java	`74.359% <0%> (+7.692%)`	`8% <0%> (+1%)`	⬆️

nh13 · 2018-09-18T23:18:58Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

@@ -23,6 +23,7 @@
    private long lastStartTime = -1;
    private String lastChrom = null;
    private int lastPos = 0;
+    private String lastRN = null;


lastReadName

nh13 · 2018-09-18T23:19:16Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

+            rnInfo = ", last read name: " + lastRN;
+        } else {
+            rnInfo = "";
+        }


formatting not consistent with the rest of the code

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

nh13 · 2018-09-18T23:23:50Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

        final long n = (this.processed % this.n == 0) ? this.n : this.processed % this.n;

        log(this.verb, " ", processed, " " + noun + ".  Elapsed time: ", elapsed, "s.  Time for last ", fmt.format(n),
-                ": ", period, "s.  Last read position: ", readInfo);
+                ": ", period, "s.  Last read position: ", readInfo, rnInfo);


sometimes read names are really long, or we don't want them in the log, can we just turn this off?

I can make a protected alternate constructor with the bool explicit and/or a setLogReadNames method, but I'm trying not to mess with the ProgressLoggerInterface so it'd only be available to subclasses of AbstractProgressLogger.

IMO this should roll out by default and folks can turn it off if they specifically object. I'll let the hivemind chime in if there are objections to more-useful-by-default

perhaps the logger can figure out if the records are sorted or not, and only emit readnames when they are not?

it doesn't do that for chr/pos

I will also vote for default to a switch for this feature, and default to off (to keep the previous behaviour).

@helgridly: I think that if you add the feature that the logger will output readname only after it notices that the records are not coordinate-sorted, this will be a good compromise between, providing information and terseness.

providing a switch would require adding yet another Default setting to HTSJDK which is gross.

Cannot be done on construction?

codecov-io · 2018-09-19T01:03:02Z

Codecov Report

Merging #1180 into master will decrease coverage by <.001%.
The diff coverage is 62.5%.

@@               Coverage Diff               @@
##              master     #1180       +/-   ##
===============================================
- Coverage     68.398%   68.398%   -<.001%     
- Complexity      8013      8015        +2     
===============================================
  Files            541       541               
  Lines          32688     32694        +6     
  Branches        5528      5529        +1     
===============================================
+ Hits           22358     22362        +4     
- Misses          8125      8128        +3     
+ Partials        2205      2204        -1

Impacted Files	Coverage Δ	Complexity Δ
...a/htsjdk/samtools/util/AbstractProgressLogger.java	`41.379% <62.5%> (+0.995%)`	`7 <1> (+1)`	⬆️
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
...va/htsjdk/samtools/sra/SRAIndexedSequenceFile.java	`64.865% <0%> (+2.703%)`	`7% <0%> (+1%)`	⬆️
...sjdk/samtools/util/Md5CalculatingOutputStream.java	`74.359% <0%> (+7.692%)`	`8% <0%> (+1%)`	⬆️

pshapiro4broad · 2018-09-20T15:06:03Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

@@ -60,10 +61,18 @@ private synchronized void record() {
        if (this.lastChrom == null) readInfo = "*/*";
        else readInfo = this.lastChrom + ":" + fmt.format(this.lastPos);

+        final String rnInfo;


Why not use readName instead of rn here? That would match what the rest of the code here does. Also, I believe that abbreviations like this make the code harder to read.

pshapiro4broad · 2018-09-20T15:10:13Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

@@ -60,10 +61,18 @@ private synchronized void record() {
        if (this.lastChrom == null) readInfo = "*/*";
        else readInfo = this.lastChrom + ":" + fmt.format(this.lastPos);

+        final String rnInfo;
+        if(lastReadName != null) {


Regarding the previous comments about formatting, I think we should be pushing the code to a consistent style based on the Sun/Oracle java style guidelines. Reformatting non-conforming code (such as the if on line 61) is preferable to committing new code that doesn't conform.

So here you'd add a space after if and delete the newline before else. IMO this is also an OK place to use a ternary op, but that's up to you.

magicDGS

Some comments

magicDGS · 2018-09-22T09:50:25Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

@@ -80,10 +89,10 @@ public synchronized boolean log() {
        }
    }

-    @Override
-    public synchronized boolean record(final String chrom, final int pos) {
+    protected synchronized boolean record(final String chrom, final int pos, final String rname) {


Why not making this public API also in the ProgressLoggerInterface? Instead of read name, it can also log record names (might be useful for some other data sources). For the interface, you can use a default value not to break compatibility with other implementations...

this is clearly scope-creep. I would hold-off on requesting that from the OP

magicDGS · 2018-09-22T09:51:31Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

@@ -60,10 +61,18 @@ private synchronized void record() {
        if (this.lastChrom == null) readInfo = "*/*";
        else readInfo = this.lastChrom + ":" + fmt.format(this.lastPos);

+        final String rnInfo;
+        if(lastReadName != null) {
+            rnInfo = ".  Last read name: " + lastReadName;


I would like this to say "record" instead of "read", to be able to log non-reads record names.

magicDGS · 2018-09-22T09:54:01Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

        final long n = (this.processed % this.n == 0) ? this.n : this.processed % this.n;

        log(this.verb, " ", processed, " " + noun + ".  Elapsed time: ", elapsed, "s.  Time for last ", fmt.format(n),
-                ": ", period, "s.  Last read position: ", readInfo);
+                ": ", period, "s.  Last read position: ", readInfo, rnInfo);


I will also vote for default to a switch for this feature, and default to off (to keep the previous behaviour).

lbergelson

I think the idea of detecting if it's ordered by position and outputting read names only iff it's not seems reasonable. I've added my own helpful nitpicking formatting suggestion as well.

lbergelson · 2018-09-19T21:20:35Z

src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java

@@ -60,10 +61,18 @@ private synchronized void record() {
        if (this.lastChrom == null) readInfo = "*/*";
        else readInfo = this.lastChrom + ":" + fmt.format(this.lastPos);

+        final String rnInfo;
+        if(lastReadName != null) {


This is nitpicky, but I'd probably make this block into a one liner since it's really just an assignment.

final String readNameInfo = lastReadName == null ? "" : ". Last read name: " + lastReadName;

lbergelson · 2019-02-25T20:00:56Z

Closing this since @yfarjoun has taken it over as #1302

progress logger logs read name too

55ffe26

nh13 requested changes Sep 18, 2018

View reviewed changes

nilspicks

7300d76

pshapiro4broad reviewed Sep 20, 2018

View reviewed changes

magicDGS reviewed Sep 22, 2018

View reviewed changes

samtools deleted a comment from codecov-io Sep 24, 2018

yfarjoun added the Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond label Sep 24, 2018

lbergelson reviewed Sep 24, 2018

View reviewed changes

yfarjoun self-assigned this Feb 11, 2019

yfarjoun mentioned this pull request Feb 22, 2019

Proposed replacement for progress PR #1302

Merged

lbergelson closed this Feb 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

progress logger logs read name too #1180

progress logger logs read name too #1180

helgridly commented Sep 18, 2018

codecov-io commented Sep 18, 2018

codecov-io commented Sep 18, 2018

codecov-io commented Sep 18, 2018

nh13 Sep 18, 2018

nh13 Sep 18, 2018

nh13 Sep 18, 2018

helgridly Sep 19, 2018 •

edited

Loading

yfarjoun Sep 20, 2018

helgridly Sep 20, 2018

magicDGS Sep 22, 2018

yfarjoun Sep 24, 2018

yfarjoun Sep 24, 2018

magicDGS Sep 25, 2018

codecov-io commented Sep 19, 2018

pshapiro4broad Sep 20, 2018

yfarjoun Sep 24, 2018

pshapiro4broad Sep 20, 2018

magicDGS left a comment

magicDGS Sep 22, 2018

yfarjoun Sep 24, 2018

magicDGS Sep 22, 2018

magicDGS Sep 22, 2018

lbergelson left a comment

lbergelson Sep 19, 2018

lbergelson commented Feb 25, 2019

progress logger logs read name too #1180

progress logger logs read name too #1180

Conversation

helgridly commented Sep 18, 2018

Description

Checklist

codecov-io commented Sep 18, 2018

Codecov Report

codecov-io commented Sep 18, 2018

Codecov Report

codecov-io commented Sep 18, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helgridly Sep 19, 2018 • edited Loading

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

codecov-io commented Sep 19, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicDGS left a 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

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson commented Feb 25, 2019

helgridly Sep 19, 2018 •

edited

Loading