-
-
Notifications
You must be signed in to change notification settings - Fork 19.1k
BUG: IntervalIndex.unique() only contains the first interval if all interval borders are negative #61920
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: main
Are you sure you want to change the base?
Conversation
Hello @khemkaran10, I have tried your fix and I have the following issue. When i run with this branch this code: Gould you try to run the code again and assure, that idx_neg.unique() really returns "IntervalIndex([(-4, -3], (-3, -2], (-2, -1]], dtype='interval[int64, right]')"? In my tests the return is "IntervalIndex([([-4607182418800017408, -4609434218613702656], (-4609434218613702656, -4611686018427387904], (-4611686018427387904, -4616189618054758400]], dtype='interval[int64, right]')" which triggers the error: "E ValueError: left side of interval must be <= right side" |
@il1sf4 Thanks for pointing out. I have update the PR. |
pandas/core/arrays/interval.py
Outdated
)._from_sequence(nc[:, 1], dtype=dtype) | ||
else: | ||
assert isinstance(dtype, np.dtype) | ||
nc = np.hstack( |
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.
is the hstack really necessary here since the next 2 lines is just splitting them back up?
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.
thanks @jbrockmendel , It's not required. I'll change this.
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on 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.
Issue is fixed, approved.
Dear reviewers @jbrockmendel , @mroeschke , please, check again the bugfix commits and if you have no other findings, please, approve the PR. I need this patch to be released ASAP for my project. |
Why is taking so long this bugfix PR to be merged? |
@mroeschke is there anything blocking this? This fix is needed here. Anything that we can do to get this merged? |
@khemkaran10 are the last two comments by @jbrockmendel resolved? |
@johannes-mueller yes both are resolved. |
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.
Gentle ping @jbrockmendel
@rhshadrach @jbrockmendel I have made the requested changes. I think it's good to merge now. |
pandas/core/arrays/interval.py
Outdated
nc = unique(self._combined) | ||
# Ensure nc is a numpy array for _from_combined | ||
if not isinstance(nc, np.ndarray): | ||
nc = np.asarray(nc) |
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.
Is this hit by the tests?
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.
@rhshadrach I added this check to handle mypy failure. should I add a separate test for this one?
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.
Is this check actually needed? I thought np.asarray()
is doing such a check internally.
Fixes: #61917
Before FIx ❌:
After Fix ✅:
Fixes incorrect deduplication of negative-valued intervals when using .unique().
Previously used .view("complex128"), which failed for negative floats/ints.
closes BUG:
IntervalIndex.unique()
only contains the first interval if all interval borders are negative #61917Tests added and passed if fixing a bug or adding a new feature
All code checks passed.