-
Notifications
You must be signed in to change notification settings - Fork 523
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
graph: use slices package for sorting and reversing slices #1957
base: master
Are you sure you want to change the base?
Conversation
403c650
to
7110b3b
Compare
As discussed in #1956 (comment), I'll more than happy to check if new benchmarks are needed first. |
This needs to pick up the changes in master. |
7110b3b
to
20c5128
Compare
Done. I'll pick them up again if #1953 is merged in. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1957 +/- ##
==========================================
+ Coverage 71.46% 72.38% +0.92%
==========================================
Files 489 557 +68
Lines 54288 65108 +10820
==========================================
+ Hits 38797 47130 +8333
- Misses 13086 15376 +2290
- Partials 2405 2602 +197 ☔ View full report in Codecov by Sentry. |
20c5128
to
b38f902
Compare
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.
Please reorder the commits so that the benchmarks are introduced first and then show the perf impacts of the change.
1951a3f
to
60d4652
Compare
Okay, commits re-ordered! I intend to rebase and run the benchmarks as soon as prerequisite PR #1953 is merged in. |
60d4652
to
357673e
Compare
f7672bf
to
8ee84f7
Compare
029bb6e
to
3fca49f
Compare
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.
What are the benchmarking outcomes from the non-coloring.Set
changes?
Thanks for checking! I'm not finished with my benchmarks yet. I ran them last night, but I made a mistake where I ran them on this branch and |
3fca49f
to
44b16ea
Compare
Here are the benchmarks. I'm not sure what to make of them. WDYT? Benchmarks
|
Yeah, that looks like a wash. I think we're fine then. I'll take another proper look on the weekend. |
This PR introduces a modernization change that is not necessarily related to #617 but which was spun out from my attempts at generifying the leaves of
graph
. Benchmarks to follow.Blocked by #1940 and #1953.