-
Notifications
You must be signed in to change notification settings - Fork 598
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.
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
fix read count report #5883
fix read count report #5883
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5883 +/- ##
===============================================
- Coverage 86.843% 86.838% -0.005%
- Complexity 32321 32323 +2
===============================================
Files 1991 1991
Lines 149308 149327 +19
Branches 16486 16483 -3
===============================================
+ Hits 129663 129672 +9
- Misses 13630 13647 +17
+ Partials 6015 6008 -7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better structure!
Some suggestions on possible further improvements.
I'd recommend also providing a more descriptive commit message.
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
Show resolved
Hide resolved
if ( report1.getRefCoverage().isEmpty() ) return report2; | ||
if ( report2.getRefCoverage().isEmpty() ) return report1; | ||
return new PairedReadReport(report1, report2); | ||
@VisibleForTesting static void combineReports( final ReadReport report1, final ReadReport report2 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I understand that this absorbs this the functionality of the old PairedReadReport.updateCounts(...)
.
I find this new architecture a little inconsistent: before this PR, there's the class ReadReport
, and PairedReadReport
, which now are merged into a single final
class ReadReport
. Now in this new ReadReport
, there's a ctor that takes in two ReadReport
's. A reader would be confused by these two: why are there two functions essentially having the same signature (sure, return types are different)?
So I'd recommend looking for a solution to absorb this into the class ReadReport
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function figures out which set of moleculeCounts to manipulate and passes that to the updateCounts function of the correct ReadReport (which isn't always a combined report). The reports are combined only in the case where they're not empty and they overlap. It's the parent class that has things chopped up into different buckets of molecule counts, and so it has a function that looks at the reports and chooses the right bucket. It's not really a concern of the ReadReport, which just updates whichever moleculeCount its given. Again, it's a separation of concerns thing. Sorry, but I like it this way better than pushing it all into the ReadReport constructor, which would then have to know about all the buckets and choose the right one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe combineReports should be renamed to updateCountsForPair? Would that be better?
Yep, I think this name reflects the intention better.
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a couple of small changes, but pushed back on two of your suggestions. I'll improve the commit description when I rebase and squash. (I'll retain only the comment on this 3rd commit.)
Let me know if you can live with it.
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
Outdated
Show resolved
Hide resolved
if ( report1.getRefCoverage().isEmpty() ) return report2; | ||
if ( report2.getRefCoverage().isEmpty() ) return report1; | ||
return new PairedReadReport(report1, report2); | ||
@VisibleForTesting static void combineReports( final ReadReport report1, final ReadReport report2 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function figures out which set of moleculeCounts to manipulate and passes that to the updateCounts function of the correct ReadReport (which isn't always a combined report). The reports are combined only in the case where they're not empty and they overlap. It's the parent class that has things chopped up into different buckets of molecule counts, and so it has a function that looks at the reports and chooses the right bucket. It's not really a concern of the ReadReport, which just updates whichever moleculeCount its given. Again, it's a separation of concerns thing. Sorry, but I like it this way better than pushing it all into the ReadReport constructor, which would then have to know about all the buckets and choose the right one.
Maybe combineReports should be renamed to updateCountsForPair? Would that be better? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically only one meaningful comment about using a different name for combineReports(...)
.
👍
724270b
to
9a68f43
Compare
Thanks very much for the review, Steve. |
No description provided.