[mayur-dhavan]

Summary

IntervalDict.combine on master (after 8db341e, the unreleased 2.7.0-pre1 performance rewrite) can silently drop pieces of the intersection of the two domains.

When a non-atomic key's interior gap is filled by a later key, the new two-pointer sweep iterates d.items() (non-atomic keys) and advances one side based on upper bounds, so it can exhaust one iterator before visiting the remaining keys of the other side. The affected points simply vanish from the combined result.

Reproducer

import portion as P

d1 = P.IntervalDict([
    (P.closedopen(3, 4) | P.openclosed(4, 5), "sides"),
    (P.singleton(4), "middle"),
])
d2 = P.IntervalDict([(P.open(3, 5), "right")])

combined = d1.combine(d2, lambda x, y: (x, y))
print(combined.get(4))  # master: None; expected ("middle", "right")

The same state is reachable through plain assignments: setting [3,4) and (4,5] to the same value merges them into a single non-atomic key, and assigning [4] afterwards fills the gap.

This is a regression: at 5f91bf5 the same code returns ("middle", "right") correctly.

Fix

Per maintainer preference, this restores the pairwise intersection loop used before 8db341e (the same implementation as 5f91bf5): the stored keys of both restricted domains are iterated directly, and how is invoked exactly once for each pair of intersecting stored keys, with the full (possibly non-atomic) intersection. This preserves the documented behavior ("f(x, y) for x in d1, y in d2 for intersecting keys") and the pre-rewrite semantics for pass_interval=True: interval-dependent callbacks receive the full intersection of a key pair rather than individual atomic pieces.

This gives up the sweep optimization in favor of the previous O(n*m) pairwise approach, as discussed in review.

Tests

Three regression tests:

• test_combine_non_atomic_interval_with_interior_piece (both directions): fails on master, passes with the fix.
• test_combine_pass_interval_non_atomic_keys and test_combine_pass_interval_non_atomic_spanning_keys: with non-atomic keys, how is invoked exactly once per intersecting key pair and receives the full union of the intersection. These pass on master and with the fix.

The full test suite passes: 233 passed, 11 skipped.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[Copilot]

🟡 Changes recommended

Atomic splitting changes existing results for interval-dependent callbacks using pass_interval=True.

1 open issue

• Preserve union intervals for pass_interval callbacks · New

_{Review effort: Balanced}

[AlexandreDecan]

Hello,

Thanks for reporting this issue. I'm a bit afraid that, with this fix, the code for combine is becoming even more complex and will be hard to maintain in the future. I'm not sure it's worth the speed-up :-/

[mayur-dhavan]

Thanks for the feedback. I can simplify this by restoring the previous pairwise intersection loop and keeping the regression tests. That version passes all 233 tests locally, but gives up the sweep's speed-up. Would you prefer that approach?

[AlexandreDecan]

Let's do it step by step :-) I definitely want the tests to be included in our test suite (we never have enough tests anyway ;-) and then I'll have a deeper look at why we get this bug and identify our options :-)

[AlexandreDecan]

I can simplify this by restoring the previous pairwise intersection loop and keeping the regression tests. That version passes all 233 tests locally, but gives up the sweep's speed-up. Would you prefer that approach?

For now, I would say yes, let's go back to the previous approach (pairwise intersection loop) to stay on the safe side. Then, we can discuss on how to speed-up combine (if needed) while keeping it "quite simple" :-)

[mayur-dhavan]

Updated as requested: restored the previous pairwise intersection loop and kept all three regression tests. All 233 tests pass locally. Thanks for the guidance.

[AlexandreDecan]

Thanks a lot!