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

Optimise ranges.any_overlapping #184

Merged
merged 7 commits into from
Jan 13, 2025

Conversation

Peter554
Copy link
Contributor

@Peter554 Peter554 commented Jan 10, 2025

This PR optimises the ranges.any_overlapping function. It uses pytest-benchmark to measure the improvement. The resulting function is >100x faster for this benchmark..!

Kraken: This function is often used to determine whether there are overlaps in meter readings etc and has a significant impact on costing performance.

-------------------------------------------------------------------------------------------- benchmark: 2 tests --------------------------------------------------------------------------------------------
Name (time in ms)                            Min                 Max                Mean            StdDev              Median               IQR            Outliers       OPS            Rounds  Iterations
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_any_overlapping (NOW)                1.4645 (1.0)        1.5542 (1.0)        1.4730 (1.0)      0.0099 (1.0)        1.4699 (1.0)      0.0032 (1.0)         70;93  678.8814 (1.0)         641           1
test_any_overlapping (0004_ce35b5e)     191.0346 (130.44)   207.6654 (133.61)   194.7505 (132.21)   6.6148 (666.40)   191.3241 (130.16)   4.8863 (>1000.0)       1;1    5.1348 (0.01)          6           1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

@Peter554 Peter554 self-assigned this Jan 10, 2025
@Peter554 Peter554 force-pushed the optimise-xocto-ranges-any-overlaps-any-gaps branch from 3eeb353 to b09a2f6 Compare January 10, 2025 16:24
@Peter554 Peter554 marked this pull request as ready for review January 10, 2025 16:26
Copy link

@romainletendart romainletendart left a comment

Choose a reason for hiding this comment

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

Looks good to me, well done! 👏

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## Unreleased

- Optimise the `ranges.any_overlapping` function [#184](https://github.com/octoenergy/xocto/pull/184).

Choose a reason for hiding this comment

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

Suggested change
- Optimise the `ranges.any_overlapping` function [#184](https://github.com/octoenergy/xocto/pull/184).
- Improve the performance of the `ranges.any_overlapping` function (with some benchmark showing a >100x speed up) [#184](https://github.com/octoenergy/xocto/pull/184).

or any similar phrasing would give a better idea of the actual change.

xocto/ranges.py Outdated
return True
range_set.add(range)
stack: list[Range[T]] = []
for range in sorted(ranges):
Copy link
Member

Choose a reason for hiding this comment

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

The tests don't cover non-consecutive overlapping ranges which is important to highlight that this sorted() call is necessary.

class TestAnyOverlapping:
    ...

    @pytest.mark.parametrize(
        "ranges_",
        [
            [
                ranges.Range(0, 2),
                ranges.Range(4, 5),  # Added this to ensure the overlapping ranges are not adjacent.
                ranges.Range(1, 3),
            ],
            [
                ranges.Range(
                    0, 2, boundaries=ranges.RangeBoundaries.INCLUSIVE_INCLUSIVE
                ),
                ranges.Range(2, 4),
            ],
        ],
    )
    def test_returns_true_if_and_ranges_overlap_non_consecutive(self, ranges_):
        assert ranges.any_overlapping(ranges_)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes nice, added

xocto/ranges.py Outdated
Comment on lines 1057 to 1067
stack: list[Range[T]] = []
for range in sorted(ranges):
if len(stack) == 0:
stack.append(range)
else:
if stack[-1].intersection(range):
return True
else:
stack.append(range)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to keep a stack as we're only looking at the last item:

Suggested change
stack: list[Range[T]] = []
for range in sorted(ranges):
if len(stack) == 0:
stack.append(range)
else:
if stack[-1].intersection(range):
return True
else:
stack.append(range)
prev_range: Range[T] | None = None
for range in sorted(ranges):
if prev_range is not None and prev_range & range:
return True
prev_range = range

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! That even helped the performance a little more 👍

@Peter554 Peter554 force-pushed the optimise-xocto-ranges-any-overlaps-any-gaps branch 2 times, most recently from 99c0097 to fcf46fb Compare January 11, 2025 07:05
@Peter554 Peter554 force-pushed the optimise-xocto-ranges-any-overlaps-any-gaps branch from fcf46fb to 2670ac0 Compare January 11, 2025 07:06
@Peter554 Peter554 merged commit 4bef5b5 into main Jan 13, 2025
1 check passed
@Peter554 Peter554 deleted the optimise-xocto-ranges-any-overlaps-any-gaps branch January 13, 2025 08:45
@Peter554 Peter554 mentioned this pull request Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants