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

ah - use new GT encoding #6822

Merged
merged 4 commits into from
Sep 16, 2020
Merged

ah - use new GT encoding #6822

merged 4 commits into from
Sep 16, 2020

Conversation

ahaessly
Copy link
Contributor

change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv

Copy link
Contributor

@meganshand meganshand left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a few questions.

if (columns == null) {
columns = line;
} else {
values = line;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there only one line in the metrics file? If so, can you put in a check for that?

@@ -30,67 +36,39 @@ public String getColumnValue(VariantContext variant, ProbeInfo probeInfo, String

// This where the validation step (required vs not) lives -- fail if there is missing data for a required field
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be deleted too?

Copy link
Contributor

@meganshand meganshand left a comment

Choose a reason for hiding this comment

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

Looks good, merge when tests pass.

Copy link
Contributor

@kcibul kcibul left a comment

Choose a reason for hiding this comment

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

Left a few comments



public static String getGTString(final VariantContext variant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm so surprised this code doesn't exist anywhere? the VCFWriter must be doing (largely) the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but it is very embedded in the actual writing of the output and in htsjdk which for some reason i think it's more difficult for us to modify to make it modular.

gt = RawArrayTsvCreator.GT_encoding.AA;
} else if (allele.basesMatch(probeInfo.alleleB)) {
gt = RawArrayTsvCreator.GT_encoding.BB;
if (alleleIndexes.size() == 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if it's not two? should we throw an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a warning

}
return gt.getValue();
return gt == RawArrayTsvCreator.value_to_drop ? "null" : gt.getValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean the string null or an actual null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the actual string "null". the tsv needs some value and the bq import will convert it to an actual null in the db. added to the comment to explain

@ahaessly ahaessly merged commit e5fc8ba into ah_var_store Sep 16, 2020
@ahaessly ahaessly deleted the ah_var_store_GTs branch September 16, 2020 16:02
meganshand pushed a commit that referenced this pull request Oct 6, 2020
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
kcibul pushed a commit that referenced this pull request Jan 29, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
kcibul pushed a commit that referenced this pull request Jan 29, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
kcibul pushed a commit that referenced this pull request Feb 1, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
kcibul pushed a commit that referenced this pull request Feb 1, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
Marianie-Simeon pushed a commit that referenced this pull request Feb 16, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
Marianie-Simeon pushed a commit that referenced this pull request Feb 16, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
kcibul pushed a commit that referenced this pull request Mar 9, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
kcibul pushed a commit that referenced this pull request Mar 9, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
mmorgantaylor pushed a commit that referenced this pull request Apr 6, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
mmorgantaylor pushed a commit that referenced this pull request Apr 6, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
mmorgantaylor pushed a commit that referenced this pull request Apr 6, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
mmorgantaylor pushed a commit that referenced this pull request Apr 6, 2021
* change GTs to single character, drop hom ref, add sample metrics to sample metadata tsv
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