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

Removed unsigned parametrization from Intervaltree #30366

Closed
wants to merge 1 commit into from

Conversation

WillAyd
Copy link
Member

@WillAyd WillAyd commented Dec 19, 2019

closes #30365 , #27169

Unclear if we currently consider this a feature, but I couldn't find anything explicitly asking for it so trying the removal route first

Not sure what the other alternative would be for parametrization, but I think generally we need to be careful when doing comparisons between signed and unsigned in templating or fused types

Side benefit - this gets rid of 128 build warnings and a slight perf boost to compilation

@jbrockmendel
Copy link
Member

this gets rid of 128 build warnings

That'd be really nice

@WillAyd
Copy link
Member Author

WillAyd commented Dec 19, 2019

Hmm missed the parametrized tests that are causing failures. Suppose this won't be as easy as I'd hoped

@jbrockmendel jbrockmendel added the Interval Interval data type label Dec 20, 2019
@WillAyd
Copy link
Member Author

WillAyd commented Dec 20, 2019

@jschendel just saw your comment here which I think is applicable:

#20651 (comment)

From quite a while ago but does this still ring a bell? Do you know what fix you had that raised an OverflowError? I actually think that would be preferable?

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you likely need to change the inner code to actually handle uint64 with different indexers

@jschendel
Copy link
Member

I don't remember exactly what I did to get the OverflowError but my vague recollection is that I had to create two additional fused types for signed vs unsigned and them add them as an additional parameter to nodes here:

nodes = []
for dtype in ['float32', 'float64', 'int32', 'int64', 'uint64']:
for closed, cmp_left, cmp_right in [

And then appropriately use the signed/unsigned fused types within the node classes.

I recall trying a lot of different things, so the above might not actually be the appropriate thing to do, and is really just the thing I most distinctly recall trying.

@WillAyd
Copy link
Member Author

WillAyd commented Jan 1, 2020

@jschendel already fixed this. closing

@WillAyd WillAyd closed this Jan 1, 2020
@WillAyd WillAyd deleted the warnings-cleanup branch January 1, 2020 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interval Interval data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IntervalIndex Comparisons of Signed / Unsigned
4 participants