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

MAX_POS too small #732

Closed
SergejN opened this issue Oct 18, 2018 · 6 comments
Closed

MAX_POS too small #732

SergejN opened this issue Oct 18, 2018 · 6 comments

Comments

@SergejN
Copy link

SergejN commented Oct 18, 2018

Dear pysam authors,

I am trying to work with a large genome, but unfortunately, I am not able to convert my BAM files to bigwig using deeptools. Digging in the code lead me to the suspicion that the problem comes from the module pysam/libchtslib.pyx. Indeed, if you check the source code you see that MAX_POS is defined as 2^29 instead of 2^31 (BAM format): Line 44: cdef int MAX_POS = 2 << 29

Is it something that can be easily fixed?
I would appreciate that. Thank you!
Sergej

@AndreasHeger
Copy link
Contributor

Certainly, this should be an easy fix.

@rembart
Copy link

rembart commented Dec 9, 2018

hey i just had a look at the fix, and without being experienced in cython it seems to me that you just oversized the int32 - I would suggest to use an unsigned int or a long for the loops
otherwise the max_pos will default to "0" since 2^32 is not a valid "positive" int32 value

i haven't tested this but please have a look

best klaus

@AndreasHeger

AndreasHeger added a commit that referenced this issue Dec 9, 2018
@AndreasHeger
Copy link
Contributor

Thanks, indeed, will need to do this more carefully on a separate branch.

@AndreasHeger AndreasHeger reopened this Dec 9, 2018
@jmarshall
Copy link
Member

The C code effectively uses (2 << 31) - 1 for this, which fits in an int32 and has the desired behaviour (modulo thinking about a couple of <= vs. < in the pysam code…).

@AndreasHeger
Copy link
Contributor

Thanks, will do the same.

@jmarshall
Copy link
Member

Uh, when I said the C code effectively uses (1 << 31) - 1, I meant it writes it as INT_MAX or 2147483647 or 0x7FFFFFFF or similar. Writing (1 << 31) - 1 overflows in C and perhaps should make you nervous in a .pyx file…

Perhaps this would be better using from libc.stdint cimport INT32_MAX as already is imported in several of these files?

jmarshall added a commit that referenced this issue Feb 28, 2021
This escaped PR #732's notice and limited samfp.fetch() (without
any arguments) to 2^29. Replace it with MAX_POS; there were no other
remaining instances of 1<<29 relating to chomosome coordinates.
Fixes #996.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants