-
Notifications
You must be signed in to change notification settings - Fork 1
updatehtslib compare against masterpull #13
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
base: masterpull
Are you sure you want to change the base?
Conversation
…left out to make test pass for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @eneskuluk. Please see comments.
@@ -1131,7 +1131,7 @@ static hFILE *hopen_unknown_scheme(const char *fname, const char *mode) | |||
} | |||
|
|||
/* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you investigate if we add to the original definition instead of replacing them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we need to be different from the origin vcf.h here. @mlathara any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -125,8 +125,10 @@ int kputd(double d, kstring_t *s) { | |||
char *z = ep+1; | |||
while (ep > cp) { | |||
if (*ep == '.') { | |||
if (z[-1] == '.') | |||
z[-1] = 0; | |||
if (z[-1] == '.'){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check formatting here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be reverted back to original, this is where it was keeping fraction part for .0 values.
vcf.c
Outdated
@@ -3487,7 +3577,7 @@ static int vcf_parse_info(kstring_t *str, const bcf_hdr_t *h, bcf1_t *v, char *p | |||
else | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as original! No need for the change.
vcf.c
Outdated
@@ -3536,6 +3626,7 @@ int vcf_parse(kstring_t *s, const bcf_hdr_t *h, bcf1_t *v) | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally same as original, we should probably remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line needs to change as well, remnant from old code.
No description provided.