GenomicsDB upgrade fixes #7257
GenomicsDB upgrade fixes #7257
Conversation
…nt with GenomicsDB
| @@ -1051,7 +1051,7 @@ public void onTraversalStart() {} | |||
| public Object onTraversalSuccess() { return null; } | |||
|
|
|||
| @Override | |||
| protected final Object doWork() { | |||
| protected Object doWork() { | |||
There was a problem hiding this comment.
I'm not a fan of making this non final. We should find a way around it by adding a mechanism to provide a different progress meter.
There was a problem hiding this comment.
Maybe just a boolean-valued method that lets you disable the progress meter would suffice here
There was a problem hiding this comment.
Reverted back to final and added a boolean valued method to disable progress meter
| logger.info("Done importing batch " + arg.batchCount + "/" + arg.totalBatchCount); | ||
| logger.info("List of samples imported in batch " + arg.batchCount + ":"); |
There was a problem hiding this comment.
I'm not sure we want to output the list of every sample in the batch? That seems very verbose.
There was a problem hiding this comment.
I actually requested this -- without a progress meter, knowing which samples you're on at least gives you some sense of where you are, and if something goes wrong you know which samples might be responsible.
There was a problem hiding this comment.
Maybe this should be debug output?
There was a problem hiding this comment.
Sure, I can make this debug if that's the consensus.
There was a problem hiding this comment.
Changed list of sample to debug
| final int sampleCount = sampleNameToVcfPath.size(); | ||
| final int updatedBatchSize = (batchSize == DEFAULT_ZERO_BATCH_SIZE) ? sampleCount : batchSize; | ||
| final int startBatch = (arg.batchCount - 1) * updatedBatchSize; | ||
| final int stopBatch = arg.batchCount * updatedBatchSize; |
There was a problem hiding this comment.
It seems like these calculations could be moved into the batch argument class itself but maybe that's not worth it since they're only used here.
There was a problem hiding this comment.
Left these since they're only used here...
| int index = 0; | ||
| final int sampleCount = sampleNameToVcfPath.size(); | ||
| final int updatedBatchSize = (batchSize == DEFAULT_ZERO_BATCH_SIZE) ? sampleCount : batchSize; | ||
| final int startBatch = (arg.batchCount - 1) * updatedBatchSize; |
There was a problem hiding this comment.
I see we decrement startBatch here but then we increment for the new batch later. Just checking to make sure we're not off by 1 in one direction.
There was a problem hiding this comment.
I don't think there is an off by 1 issue here. Minus 1 here just to get the upper and lower bounds
|
@droazen @lbergelson can you take another look? |
| * | ||
| * @return true if this tools wants to disable progress meter output, otherwise false | ||
| */ | ||
| public boolean disableProgressMeter() { |
There was a problem hiding this comment.
There are some issues with integrating this new method into the various traversals, but I will address those myself in a separate PR after this is merged.
This PR contains the GATK code necessary to enable some features present in the recent GenomicsDB update: