search

bxlab / bx-python

Tools for manipulating biological data, particularly multiple sequence alignments
MIT License
145 stars 53 forks source link

Comparing an integer to builtin function in interval_index_file.py #6

Closed jeffhsu3 closed 8 years ago

jeffhsu3 commented 8 years ago

In interval_index_file.py, BIN_OFFSETS_MAX[ 0 ] = max. Is this meant to be set as "MAX" rather than the python builtin function? Also the indent level in offsets_for_max_size seems to be wrong.

jxtx commented 8 years ago

Hmmm, seems like it should be. I got that line from Bob Harris (not on github?) I think -- https://github.com/bxlab/bx-python/commit/df7d682ec62ee5f36f01b67e722ac7d8c6ff3d90

jeffhsu3 commented 8 years ago

Changing it to MAX seems to work. I'm also trying to make bx-python python 3 compatible. Not sure how to handle convert 'str(key)' into bytes for the indexes.

rsharris commented 8 years ago

I'm not sure how much of that module I wrote, but ... based on the comments it should be setting that to MAX (but see the caveat in my next paragraph). MAX has no value until a few lines later though. So I think the definitions of MIN and MAX (lines 121-126) should be moved up, right after VERSION = 2.

As the comment indicates, the max position is a signed integer. The loop that filled BIN_OFFSETS_MAX puts 2^32 into bin 0, so the offending line is intended to correct that to 2^31. I'm not sure whether it ought to be 2^31-1 instead. Even if the intervals being processed are half open (open on the right), if the position values are truly limited to signed integers, there's no possibility of an interval that has 2^31 as an end (assuming the comment is correct about signed integers). I'm not familiar enough with the data structure at this point to know whether this matters.

Also worth noting that lower case min and max are used as argument names in the Index and Indexes classes. Don't know if I wrote that or not -- seems like bad practice if I did.

nsoranzo commented 8 years ago

I also noticed that and changed it to sys.maxsize in PR #7 (sys.maxint has been removed in Python 3.1 since there is no longer a limit to the value of integers).

nsoranzo commented 8 years ago

This has been fixed in #7.

jxtx commented 8 years ago

Closed per @nsoranzo