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

PERF: optimize cylindrical pixelizer routine #5035

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Oct 30, 2024

PR Summary

Yet another alternative to #5018 and #5034
This separates filtering of pixels that only partially intersect with the domain and aims to do a minimal amount of work there. On my system I'm getting similar performances as compared to #5034, but the goal here is to not change the observable behavior. Let's see how this does in CI 🤞🏻

@neutrinoceros
Copy link
Member Author

I realized that there's still a defect with my approach: I'm only shaving the pixilization from the outside in, missing most (if not all) pixels on the inside of the radial domain. I think I can figure this out too but it'll take more time than I have left today, so it'll wait for another day. I also don't think we have any image test that would reveal this bug as we use rmin=0 everywhere. I think we should start by adding a test case for this.

I also see a segfault on windows, which is probably not actually platform specific, and needs to be addressed in any case.

@neutrinoceros
Copy link
Member Author

I fixed the segfault, which was actually trivial to locate by re-enabling bound checking for debugging.

@neutrinoceros
Copy link
Member Author

Adding the missing tests in #5036

@neutrinoceros neutrinoceros force-pushed the perf/optimize_cylindrical_pixelizer branch from ad86889 to 9bc3228 Compare October 31, 2024 17:54
@neutrinoceros
Copy link
Member Author

rebased with #5036 merged. I expect one of the two new image tests to fail.

@neutrinoceros neutrinoceros force-pushed the perf/optimize_cylindrical_pixelizer branch from 4553634 to 9bc3228 Compare October 31, 2024 18:38
@neutrinoceros
Copy link
Member Author

Hum, I haven't been able to figure out a trick to fix "pixel shaving" on the inner side without compromising performance yet, so I'm starting to think it'd be preferable to go with #5018 first and revisit this after the 4.4.0 release. Does this suite you, @chrishavlin ?

@chrishavlin
Copy link
Contributor

Ya, that's fine by me! We can also hold #5018 to after 4.4.0 if that would make the effort in this PR easier.

@neutrinoceros neutrinoceros force-pushed the perf/optimize_cylindrical_pixelizer branch from 9bc3228 to 5e8b31e Compare November 11, 2024 09:58
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.

2 participants