-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
BLD: Fix IntervalTree build warnings #30560
Conversation
elif dtype.startswith('uint'): | ||
fused_prefix = 'uint_' | ||
elif dtype.startswith('float'): | ||
fused_prefix = '' |
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.
open to a better naming convention for the fused types to make this less gross
self.root.query(result, target[i]) | ||
try: | ||
self.root.query(result, target[i]) | ||
except OverflowError: |
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.
Need to add these as operations like IntervalTree[uint64].get_indexer([-1])
will raise an OverflowError
. See the associated tests I added.
int64_t | ||
int32_t | ||
float64_t | ||
float32_t |
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 think we can drop support for float32
and int32
dtypes here, which should help reduce build time. There is no practical way to actually get an IntervalTree
of these dtypes since IntervalIndex
is backed by 2 indexes and we don't have a Float32Index
or Int32Index
. Will save this for a followup though.
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.
Soounds good.
im confused by the naming here: why are the float dtypes included in int_scalar_t/uint_scalar_t?
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 couldn't think of a good way to name those. Idea is that int_scalar_t
/uint_scalar_t
are things that are comparable to int/uint, and we want to be able to compare against float in both cases to determine things like 0.5 being in the interval (0, 1).
I tried it without including floats and got "no matching type signature" errors when trying stuff like IntervalTree[int].get_indexer([0.5])
.
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.
Wouldn’t comparisons to float be very inaccurate in the 2 ** 63 plus range where we get unsigned?
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.
Yes, comparisons will be inaccurate above 2**61
, so the issue is also present on the upper/lower ends of the int64 range. Comparisons are still valid below this so probably something we'll have to live with, e.g. the same holds for comparisons against UInt64Index
but the behavior is still allowed there.
thanks @jschendel |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
xref #30366
cc @WillAyd