Skip to content
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

[AoU DRC] Support uppercase site_ids for reblocking #7929

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

mmorgantaylor
Copy link
Member

This PR modifies the AoU-specific WDL GvsAoUReblockGvcf to support processing samples whose site_ids (usually lowercase) were sent in as uppercase.

.dockstore.yml Outdated
@@ -73,6 +73,7 @@ workflows:
branches:
- master
- ah_var_store
- mmt_aou_site_id_fix
Copy link
Member Author

Choose a reason for hiding this comment

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

to remove before merging, but keeping in case changes are requested

(if select_first([site_id]) == "bcm" then "gs://prod-genomics-data-baylor/" else
(if select_first([site_id]) == "uw" then "gs://prod-genomics-data-northwest/" else "null" )))
else ""
# janky lower
Copy link
Member Author

Choose a reason for hiding this comment

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

please someone tell me a better way to do this

Copy link

Choose a reason for hiding this comment

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

I couldn't find a way. If no one else can, I would just add a comment that this is WDL for .toLowerCase().

Copy link
Member Author

Choose a reason for hiding this comment

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

good call, thanks!

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (ah_var_store@874d615). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             ah_var_store     #7929   +/-   ##
================================================
  Coverage                ?   86.288%           
  Complexity              ?     35194           
================================================
  Files                   ?      2170           
  Lines                   ?    164888           
  Branches                ?     17785           
================================================
  Hits                    ?    142278           
  Misses                  ?     16288           
  Partials                ?      6322           

Copy link

@rsasch rsasch left a comment

Choose a reason for hiding this comment

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

No help, just a nit.

(if select_first([site_id]) == "bcm" then "gs://prod-genomics-data-baylor/" else
(if select_first([site_id]) == "uw" then "gs://prod-genomics-data-northwest/" else "null" )))
else ""
# janky lower
Copy link

Choose a reason for hiding this comment

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

I couldn't find a way. If no one else can, I would just add a comment that this is WDL for .toLowerCase().

@@ -77,7 +81,7 @@ task ReblockAndCopy {
set -x

if [ ~{site_id} -a ~{dir} == "null" ]; then
echo "dir is not set to a valid value: ~{dir}. check site_id - only valid values are ['bi', 'bcm', 'uw']"
echo "dir is not set to a valid value. site_id is ~{site_id}. check site_id - only valid values are ['bi', 'bcm', 'uw']"
Copy link

Choose a reason for hiding this comment

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

nit for clarity (feel free to finesse)

Suggested change
echo "dir is not set to a valid value. site_id is ~{site_id}. check site_id - only valid values are ['bi', 'bcm', 'uw']"
echo "The additional output directory for the reblocked gVCFs can not set to a valid value because the site_id is '~{site_id}' and the only valid values are ['bi', 'bcm', 'uw']."

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you! this is way better

else ""
# this is WDL for .toLowerCase() for the three possible site codes
String site_id_lower = if defined(site_id) then (
sub(sub(sub(select_first([site_id]), "BI", "bi"), "BCM", "bcm"), "UW", "uw")
Copy link
Contributor

Choose a reason for hiding this comment

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

omg this hurts, but I understand

Copy link
Member Author

Choose a reason for hiding this comment

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

i agree, i am embarrassed

@mmorgantaylor mmorgantaylor merged commit 4b2cf4b into ah_var_store Jul 7, 2022
@mmorgantaylor mmorgantaylor deleted the mmt_aou_site_id_fix branch July 7, 2022 18:00
This was referenced Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants