-
Notifications
You must be signed in to change notification settings - Fork 575
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
Dataflow BaseRecalibrator, local only #523
Conversation
/** | ||
* For each input block, compute its statistics. | ||
* <p> | ||
* Basically just delegate to the CalibrationTablesBuilder. |
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 method seems very long and complex for something that 'delegates to CalibrationTablesBuilder'
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 can add that it also downloads the reference and prints statistics to the DF logs.
this is pretty scary looking code and it's not tied with the dataflow 'walker' interface and so i'm assuming it's just a temporary step to putting all of this in and start refactoring. fine to live in the dev package for now. It will show up on the commandline though - maybe we need a way to hide those dev tools. |
7172d13
to
cfdaca8
Compare
This PR has been open 6 days - I'd love to see it resolved. Hoping that a 1:1 conversation David R & JP can resolve the remaining issues quickly. |
well, JP just pushed the code addressing the review last night. I'm looking at it now too. We're working under the assumption that all of this code is going to go through an overhaul anyway I guess. |
done with my review. looks fine for merge to dev. Once David+David put their code in and my MarkDups is in we need to restructure all of this. |
858113b
to
2127685
Compare
Merging as soon as Travis gives the OK. |
Dataflow BaseRecalibrator, local only
(this depends on PR#522)