Skip to content

Conversation

dmiller15
Copy link
Contributor

@dmiller15 dmiller15 commented Apr 25, 2025

Description

Feel free to ignore the packed workflow in the review. It's just there to expedite workflow updating for ICA.

Made the following changes:

  • Updated the CNV blacklist suggested input to be the BED file. The old interval file did not work with the prepare_regions workflow
  • Updated the workflow to be cwltool compatible/ICA compatible

Part of https://d3b.atlassian.net/browse/BIXU-3907

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test Configuration:

  • Environment:
  • Test files:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • I have committed any related changes to the PR

@dmiller15 dmiller15 self-assigned this Apr 29, 2025
@dmiller15 dmiller15 added bug Something isn't working enhancement New feature or request documentation Adds documentation bix-dev This issue or pull request is bix-dev work labels Apr 29, 2025
@dmiller15 dmiller15 marked this pull request as ready for review April 29, 2025 18:34
Copy link
Member

@migbro migbro left a comment

Choose a reason for hiding this comment

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

Looks mostly good, have a couple areas in the comments where I'd like some clarification before approving

when: $(inputs.input[3] != null)
when: $(inputs.run_step != null)
in:
run_step: supplement_vcfs
Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, kind of doing like a variable assignment for clarity instead of referring to an array position, nice

}
if (inputs.flag != "N"){
cmd += "bedtools intersect -a " + inputs.input_vcf.path + " -b " + inputs.input_bed_file.path + " -header -wa > " + out_vcf + " && ";
cmd += "bedtools intersect -a " + (inputs.input_vcf ? inputs.input_vcf.path : "") + " -b " + inputs.input_bed_file.path + " -header -wa > " + out_vcf + " && ";
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, if inputs.input_vcf is null, then give empty string to -a? How does that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to get around a "null has no attribute path" error. In reality if the input_vcf is not provided, we'll have exited anyway.

if (ary != null){
for(var i = 0; i < ary.length; i++) {
if(Array.isArray(ary[i])) {
ret = ret.concat(flatten(ary[i]));
Copy link
Member

Choose a reason for hiding this comment

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

love a recursion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dangerous if we have infinitely nested lists 🐰


}
}
return ''
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 return com?

source: [strelka2/strelka2_protected_outputs, mutect2/mutect2_protected_outputs, lancet_input_vcf]
valueFrom: |
$(self.filter(function(e) { return e != null }).map(function(e) { return e.filter(function(i) { return i.basename.search(/(.vcf|.vcf.gz)$/) != -1 }) }).reduce(function(a,c) { return a.concat(c) }))
$(self.filter(function(e) { return e != null }).map(function(e) { return e.filter(function(i) { return i.basename.search(/(.vcf|.vcf.gz)$/) != -1 }) }).reduce(function(a,c) { return a.concat(c) }, []))

Choose a reason for hiding this comment

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

I always found these one-liner formats to be so difficult to read

@migbro migbro self-requested a review May 1, 2025 15:41
Copy link
Member

@migbro migbro left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bix-dev This issue or pull request is bix-dev work bug Something isn't working documentation Adds documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants