Add TrainGCNV input specifying subset list of samples for training #294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mwalker174
requested changes
Feb 10, 2022
wdl/TrainGCNV.wdl
Outdated
| @@ -111,7 +125,8 @@ workflow TrainGCNV { | |||
| } | |||
| } | |||
|
|
|||
| Array[Int] sample_indices = select_first([RandomSubsampleStringArray.subsample_indices_array, range(length(samples))]) | |||
| Array[Int] sample_indices = select_first([GetSubsampledIndices.subsample_indices_array, RandomSubsampleStringArray.subsample_indices_array, range(length(samples))]) | |||
| Array[String] sample_ids = select_first([GetSubsampledIndices.subsampled_strings_array, RandomSubsampleStringArray.subsampled_strings_array, samples]) | |||
There was a problem hiding this comment.
I think it would be simpler/clearer to move this into the scatter below:
scatter (i in sample_indices) {
String sample_ids = samples[i]
call cov.CondenseReadCounts as CondenseReadCounts {
input:
counts = count_files[i],
sample = samples[i],
num_bins = condense_num_bins,
expected_bin_size = condense_bin_size,
condense_counts_docker = condense_counts_docker,
runtime_attr_override=condense_counts_runtime_attr
}
}
There was a problem hiding this comment.
Also minor style comment, I might rename this to something making it clear it isn't an input. I usually add an extra underscore to the end, or you could call it maybe_subsetted_sample_ids for example.
wdl/Utils.wdl
Outdated
|
|
||
| RuntimeAttr default_attr = object { | ||
| cpu_cores: 1, | ||
| mem_gb: 3.75, |
There was a problem hiding this comment.
Suggested change
| mem_gb: 3.75, | |
| mem_gb: 1, |
wdl/Utils.wdl
Outdated
Comment on lines
247
to
248
| all_strings = ['~{sep="','" all_strings}'] | ||
| subset_strings = {'~{sep="','" subset_strings}'} |
There was a problem hiding this comment.
It would be more robust/scalable to use write_lines() to write the strings to a file and read them in, rather than defining them inline (who knows what would happen if we fed this 10k samples for example).
|
Updates since review:
Testing since review:
|
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates
Add
sample_ids_training_subsetinput toTrainGCNVto allow Terra users to specify a subsetted list of sample IDs on which to train the gCNV model, while keeping the same batching they will use for GatherBatchEvidence. Intended as an alternative to the random downsampling option if the user wants a greater level of control over the samples chosen.Testing
TrainGCNVontest_smalltest set and specified four sample IDs for training, out of order. Verified that the four were sorted according to the order of thesamplesinput (and other sample-level inputs) and the rest of the tasks were performed on the subset of samples.