Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

progress logger logs read name too #1180

Closed
wants to merge 2 commits into from
Closed

progress logger logs read name too #1180

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55ffe26
progress logger logs read name too
Sep 18, 2018
7300d76
nilspicks
Sep 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/main/java/htsjdk/samtools/util/AbstractProgressLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ abstract public class AbstractProgressLogger implements ProgressLoggerInterface
private long lastStartTime = -1;
private String lastChrom = null;
private int lastPos = 0;
private String lastReadName = null;

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

final String rnInfo;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if(lastReadName != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rnInfo = ". Last read name: " + lastReadName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else {
rnInfo = "";
}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@helgridly helgridly Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't do that for chr/pos

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot be done on construction?

}

/**
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.lastChrom = chrom;
this.lastPos = pos;
this.lastReadName = rname;
if (this.lastStartTime == -1) {
this.lastStartTime = System.currentTimeMillis();
}
Expand All @@ -96,17 +105,22 @@ public synchronized boolean record(final String chrom, final int pos) {
}
}

@Override
public synchronized boolean record(final String chrom, final int pos) {
return record(chrom, pos, null);
}

/**
* Records that a given record has been processed and triggers logging if necessary.
* @return boolean true if logging was triggered, false otherwise
*/
@Override
public synchronized boolean record(final SAMRecord rec) {
if (SAMRecord.NO_ALIGNMENT_REFERENCE_NAME.equals(rec.getReferenceName())) {
return record(null, 0);
return record(null, 0, rec.getReadName());
}
else {
return record(rec.getReferenceName(), rec.getAlignmentStart());
return record(rec.getReferenceName(), rec.getAlignmentStart(), rec.getReadName());
}
}

Expand Down