updatehtslib compare against masterpull #13
Conversation
…left out to make test pass for now
nalinigans
left a comment
nalinigans
left a comment
There was a problem hiding this comment.
Looks great @eneskuluk. Please see comments.
|
|
||
| /* Returns the appropriate handler, or NULL if the string isn't an URL. */ | ||
| static const struct hFILE_scheme_handler *find_scheme_handler(const char *s) | ||
| const struct hFILE_scheme_handler *find_scheme_handler(const char *s) |
There was a problem hiding this comment.
Can you see if there is a new api to use the htslib plugin mechanism for schemes like azb://.s3://, so we don't have to expose this function?
There was a problem hiding this comment.
There are new functions to list plugins and schemes. Explained here : samtools#1170 and samtools#1222. Not reviewed in depth yet.
htslib/vcf.h
Outdated
| #define BCF_HT_REAL 2 | ||
| #define BCF_HT_STR 3 | ||
| #define BCF_HT_LONG (BCF_HT_INT | 0x100) // BCF_HT_INT, but for int64_t values; VCF only! | ||
| #define BCF_HT_REAL 7 |
There was a problem hiding this comment.
Can you investigate if we add to the original definition instead of replacing them?
There was a problem hiding this comment.
Yeah, I think we can do that, keep the originals, replace what we use now in the new ones.
There was a problem hiding this comment.
I reverted changes to HT_REAL and HT_STR but HT_LONG requires more changes on our genomicsdb side(variant_operations.cc) as it is done according to being HT_LONG not that high value. I might need to take a look a bit more.
htslib/vcf.h
Outdated
| #define VCF_OVERLAP (1<<5) // overlapping deletion, ALT=* | ||
| #define VCF_INS (1<<6) // implies VCF_INDEL | ||
| #define VCF_DEL (1<<7) // implies VCF_INDEL | ||
| #define VCF_OTHER 32 |
There was a problem hiding this comment.
Not sure why we need to be different from the origin vcf.h here. @mlathara any ideas?
There was a problem hiding this comment.
Seems to work just fine with default values. Passes all tests when it is like following, all values are default except the ones we added:
#define VCF_REF 0
#define VCF_SNP (1<<0)
#define VCF_MNP (1<<1)
#define VCF_INDEL (1<<2)
#define VCF_OTHER (1<<3)
#define VCF_BND (1<<4) // breakend
#define VCF_OVERLAP (1<<5) // overlapping deletion, ALT=*
#define VCF_INS (1<<6) // implies VCF_INDEL
#define VCF_DEL (1<<7) // implies VCF_INDEL
#define VCF_ANY (VCF_SNP|VCF_MNP|VCF_INDEL|VCF_OTHER|VCF_BND|VCF_OVERLAP|VCF_INS|VCF_DEL) // any variant type (but not VCF_REF)
#define VCF_NON_REF (1<<8)
#define VCF_SPANNING_DELETION (1<<5)
kstring.c
Outdated
| if (*ep == '.') { | ||
| if (z[-1] == '.') | ||
| z[-1] = 0; | ||
| if (z[-1] == '.'){ |
There was a problem hiding this comment.
this will be reverted back to original, this is where it was keeping fraction part for .0 values.
vcf.c
Outdated
| v->rlen = val1 - v->pos; | ||
| } | ||
| } else if ((y>>4&0xf) == BCF_HT_REAL) { | ||
| } else if ((y >> 4 & 0xf) == BCF_HT_REAL) { |
There was a problem hiding this comment.
Same as original! No need for the change.
vcf.c
Outdated
| // Ensure string we parse has space to permit some over-flow when during | ||
| // parsing. Eg to do memcmp(key, "END", 4) in vcf_parse_info over | ||
| // the more straight forward looking strcmp, giving a speed advantage. | ||
| /* |
There was a problem hiding this comment.
Remove extra line as we will try to make the patch as minimal as possible.
vcf.c
Outdated
| s->s[s->l+2] = 0; | ||
| s->s[s->l+3] = 0; | ||
|
|
||
| for(int i = 0; i < 4; i++) |
There was a problem hiding this comment.
Functionally same as original, we should probably remove this.
There was a problem hiding this comment.
Those were running in the original but the we way we use it in the parse_info functions breaks it. I'll make sure to remove those.
htslib/vcf.h
Outdated
| else{ | ||
| *p++ = 1<<4|BCF_BT_INT64;// | ||
| i64_to_le(size,p); | ||
| s->l += 10; // not so sure about +10 here, whether it is accurate or changes anything. |
There was a problem hiding this comment.
Do you think this line of code accurate @nalinigans ? Not sure how x in s->l += x determined.
faidx.c
Outdated
| } | ||
|
|
||
| s[l] = '\0'; | ||
| *len = l < INT_MAX ? l : INT_MAX; |
There was a problem hiding this comment.
this line needs to change as well, remnant from old code.
No description provided.