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: restore cylindrical pixelizer performance from yt 4.0.2 (revert gh-3818) #5034

Closed
wants to merge 1 commit into from

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Oct 29, 2024

PR Summary

Reverts #3818 after identifying it as a significant performance regression (see exploration in #5018 (comment))

This may complement or replace #5018, but I currently think it should be considered for merging first.

update: diffs are expected, here's the companion PR: yt-project/yt_pytest_mpl_baseline#7

@matthewturk
Copy link
Member

So I think we should revert this, but, I'm trying to convince myself that we can't make this change also happen just for situations where the pixels are at an extrema in one of the dimensions. Like, do we have to do this for all of them? It seems like we could split the difference by computing the full "does it go beyond" only in cases where, say, we're toward the end of the theta or r iteration? Maybe that's not possible.

It also seems to me that because we're iterating on the inside around theta and on the outside around r, we could potentially use a break instead. Or move a continue somewhere with early termination.

Anyway, we don't really necessarily need to keep this behavior, like you said -- in most if not all cases the pixels will be very small compared to the cell values, so it shouldn't be that big a deal.

Actually, hm. In theory, this takes a long time because we're doing it for every cell value that gets deposited into a pixel, for every pixel. I'll think about it, because I wonder if it's possible to take all the extrema values of $(r,\theta)$ pairs and compute those, and then apply that to mask or buff afterward.

@neutrinoceros
Copy link
Member Author

I wonder if it's possible to take all the extrema values of ( r , θ ) pairs and compute those, and then apply that to mask or buff afterward.

That would be ideal, I suppose.

@matthewturk
Copy link
Member

I wonder if it's possible to take all the extrema values of ( r , θ ) pairs and compute those, and then apply that to mask or buff afterward.

That would be ideal, I suppose.

I'd propose we include this, but also include a comment pointing a possible way forward -- for those cases where sub-pixel boundaries are critical, in case someone wants to do it.

@neutrinoceros
Copy link
Member Author

Actually let me try to do it now.

@neutrinoceros neutrinoceros marked this pull request as draft October 30, 2024 13:31
@chrishavlin
Copy link
Contributor

for what it's worth I like the direction this is headed! once it's ready for review again I can try out the gil release again to see how much of an extra boost we can get.

@neutrinoceros
Copy link
Member Author

I took a slightly different approach with #5035, which is not finished yet, but I think is promising.

@neutrinoceros neutrinoceros modified the milestones: 4.4.0, 4.5.0 Nov 7, 2024
@neutrinoceros
Copy link
Member Author

closing in favor of #5035

@neutrinoceros neutrinoceros deleted the revert_3818 branch November 11, 2024 09:43
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.

3 participants