Skip to content

refactor: add Rbush/Flatbush impl in SegmentTree #105

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

akhilender-bongirwar
Copy link
Contributor

fixes #102
/claim #102

2025-04-26.16-00-44.mp4

Copy link

vercel bot commented Apr 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
capacity-node-autorouter ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2025 4:50pm

@seveibar
Copy link
Contributor

@akhilender-bongirwar nice video, clearly shows the difference! on a plane so might be a bit before merge. Can you confirm there are no new DRC errors? (DRC errors check that the implementation is correct by making sure lines don't overlap improperly)

Personally I like to always check against keyboard5 because it tends to catch any subtle bugs. tyty!!!

@seveibar
Copy link
Contributor

seveibar commented Apr 26, 2025

Keyboard 3 is also a good one if keyboard5 is too slow to run for you.

I got 7.5s on main vs 7s on your branch AWESOME /tip $10

Copy link

algora-pbc bot commented Apr 26, 2025

🎉🎈 @akhilender-bongirwar has been awarded $10 by tscircuit! 🎈🎊

Copy link

algora-pbc bot commented Apr 26, 2025

Please visit Algora to complete your tip via Stripe.

@seveibar
Copy link
Contributor

It seems like there may be performance degradation again. Compare this page with your branch? You can also slow down your CPU using the performance tab to help get worse, more measurable time

https://unraveller.vercel.app/?fixtureId=%7B%22path%22%3A%22examples%2Fsimplified-path-solver%2Fsimplifiedpathsolver8.fixture.tsx%22%7D

@akhilender-bongirwar
Copy link
Contributor Author

akhilender-bongirwar commented Apr 26, 2025

It seems like there may be performance degradation again. Compare this page with your branch? You can also slow down your CPU using the performance tab to help get worse, more measurable time

https://unraveller.vercel.app/?fixtureId=%7B%22path%22%3A%22examples%2Fsimplified-path-solver%2Fsimplifiedpathsolver8.fixture.tsx%22%7D

2025-04-27.00-44-44.mp4

@seveibar please check

@akhilender-bongirwar
Copy link
Contributor Author

@akhilender-bongirwar nice video, clearly shows the difference! on a plane so might be a bit before merge. Can you confirm there are no new DRC errors? (DRC errors check that the implementation is correct by making sure lines don't overlap improperly)

Personally I like to always check against keyboard5 because it tends to catch any subtle bugs. tyty!!!
Keyboard 3 is also a good one if keyboard5 is too slow to run for you.

Thank you. Sure, here it is

2025-04-27.00-49-43.mp4

@akhilender-bongirwar
Copy link
Contributor Author

@seveibar why aren't the workflows running?

@seveibar
Copy link
Contributor

@akhilender-bongirwar not sure, will be looking at this later today, thanks for the vid, definitely helps!!

@akhilender-bongirwar
Copy link
Contributor Author

@akhilender-bongirwar not sure, will be looking at this later today, thanks for the vid, definitely helps!!

@seveibar, works now

@seveibar
Copy link
Contributor

seveibar commented May 1, 2025

Some delays getting to this, if you can show it has a large performance impact via a side-by-side screenshot with https://unraveller.vercel.app (before/after), then I can merge, otherwise need to turn to some important cache optimizations, we will eventually get this in either way most likely!!!

@akhilender-bongirwar
Copy link
Contributor Author

  1. Keyboard3
  • Before

keyboard3_unraveller
Time taken: 23.240s

  • After

keyboard3_localhost

Time Taken: 22.854s

  1. Keyboard5
  • Before
    Keyboard5_unraveller

Time taken: 25.044s

  • After

keyboard5_localhost

Time taken: 23.532s

  1. SimplifiedPathSolver5
  • Before

simplifiedpathsolver5

Time taken: 0.232s

  • After

simplifiedpathsolver5_localhost

Time taken: 0.116s

@akhilender-bongirwar
Copy link
Contributor Author

Some delays getting to this, if you can show it has a large performance impact via a side-by-side screenshot with https://unraveller.vercel.app (before/after)

cannot tell large performance impact but the above are the before/after results. Do you want me to add any other components screenshots too?

@akhilender-bongirwar
Copy link
Contributor Author

@seveibar sorry for delay I was busy with my end semester exams. Please let me know if any further changes are required here

Signed-off-by: Akhilender Bongirwar <[email protected]>
@seveibar
Copy link
Contributor

@akhilender-bongirwar yes sorry for the delay, im going to be working to merge this in the next couple days

@akhilender-bongirwar
Copy link
Contributor Author

@seveibar please check

@akhilender-bongirwar
Copy link
Contributor Author

@seveibar please check

cc @seveibar

@seveibar
Copy link
Contributor

seveibar commented Jun 8, 2025

@akhilender-bongirwar sprry for the delay, on the issues tab we currently have some major bugs so im trying to be careful to not switch the implementation and accidentally introduce new ones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert the SegmentTree to be able to use an rbush or flattree spatial index
2 participants