search

rust-bio / rust-bio-tools

A set of command line utilities based on Rust-Bio.
MIT License
182 stars 24 forks source link

[vcf-report] 0-based position reporting for 1-based VCF file #230

Closed hassanfa closed 2 years ago

hassanfa commented 2 years ago

In my VCF file, I have a variant at the following specific position:

#CHROM  POS ID  REF ALT QUAL    FILTER  INFO    FORMAT  TUMOR
22  42524491    .   C   <DUP>   334 PASS    TYPE=DUP;AF=0.1005; GT:DP 0/1:3091

Above variant is missing HGVSg and vcf-report clearly reports it. But it reports a 0-based position. It is a minor issue, but it'd be useful to either properly have it 1-based or clearly specify what convention vcf-report is using for reporting. I prefer the former, it just makes the debugging easier.

Also, it does not report chromosome either.

Here is the vcf-report output:

thread '<unnamed>' panicked at 'Failed building table report for sample a. Found variant . at position 42524490 with no HGVSg value.', src/main.rs:207:21

Ideally the output could be something like:

thread '<unnamed>' panicked at 'Failed building table report for sample a. Found variant . at position 22:42524491 with no HGVSg value.', src/main.rs:207:21
hassanfa commented 2 years ago

Looking in the code, it seems pos is assigned but chrom is not: https://github.com/rust-bio/rust-bio-tools/blob/d1a60cc8876eb8ea2b6cffb21bdcee44a00d2dc4/src/bcf/report/table_report/create_report_table.rs#L85-L110

I'll look into the contribution guide and give this is a go.

fxwiegand commented 2 years ago

Hello @hassanfa,

thanks again for reporting this issue. Are you sure you are using the latest version? I recently updated the error messages to have the variants reported as <CHROM>:<POS>. Your error seems to be from a previous version since the : would be missing, even if chrom was empty (which i don't think it should be).

https://github.com/rust-bio/rust-bio-tools/blob/2737b1e8f5dae203e2f3874589343f143f2ad9dc/src/bcf/report/table_report/create_report_table.rs#L271

Regarding the 0-based position i absolutely agree. The reported position should be 1-based to make it much more convenient for the user to search for the problematic record. I'll tackle this in an incoming PR shortly.

hassanfa commented 2 years ago

@fxwiegand thanks for the quick fix. Yes, you are absolutely right that I was not using the latest version.

fxwiegand commented 2 years ago

@fxwiegand thanks for the quick fix. Yes, you are absolutely right that I was not using the latest version.

Ok great! Thank you though for pointing out the wrongly payed position! 😊