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

BUG: don't colorize pixels that are not completely within the data domain in pixelize_cylinder routine #3818

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Feb 19, 2022

PR Summary

This fixes an issue with the cylindrical pixelizer routine discovered in #3817, where pixels that are not completely within the data domain.
Although a bug fix, I don't think this change is really important before #3817 is merged, so I think this can go to the next feature relase and not be backported.

I'll demonstrate the change using the script from #3817
UniformGridData_Slice_phi_velocity_r

opening as a draft because this branch is based on #3817 for testing purposes

@neutrinoceros
Copy link
Member Author

The failures are expected. This changes the way we approximate a disk with square pixels: on main, it is approached from the outer side, while here it's approached from the inner side. As a result, on a 800x800 image, we're shaving of a thin band from the reference pixelizations.

@neutrinoceros
Copy link
Member Author

Actually it makes more sense to backport since we may otherwise end up with avoidable conflicts with the backport branch when answer tests are bumped. I'm rebasing this on top of the main branch.

@neutrinoceros neutrinoceros modified the milestones: 4.1.0, 4.0.3 Feb 20, 2022
@neutrinoceros neutrinoceros marked this pull request as ready for review February 20, 2022 10:41
@matthewturk
Copy link
Member

So, I have to think about this a bit more, but what I'm wondering is if we should instead attempt to set up a "dual grid" where the evaluation points are set at the centers of the pixels. Because we fill from left-right, that would mean basically setting up a grid that overlaid it (and so it would be something like size/factor + 1 I think?) rather than the buffer line setting that is done in here.

@neutrinoceros
Copy link
Member Author

what I'm wondering is if we should instead attempt to set up a "dual grid" where the evaluation points are set at the centers of the pixels.

I'm confused. Aren't evaluation points already at the centers ?

@neutrinoceros
Copy link
Member Author

This PR doesn't need resolution for 4.0.3 , I'd rather take it out of the milestone rather than having it blocking the release. That said, maybe we can resolve it anyway. I'll take another look at the existing code when I have time and see if this can be revised.

@matthewturk
Copy link
Member

On reflection, I think some of my question is addressed now that I've looked more closely at the arrows in the plot. I was seeing an asymmetry in the distance from the edge, but I think that's an artifact of my perception; the way the arrow heads are pointing makes it look like the top ones are closer to the edge than the bottom ones. I think this is OK.

@neutrinoceros
Copy link
Member Author

yes there's a top/bottom asymmetry in the cylindrical pixelizer, I never could figure out where it came from but it's not new, and not critical either in production, as far as I can tell.

@neutrinoceros
Copy link
Member Author

@matthewturk just to be sure, can I bump answers for this one ? I also need to refresh the logs: @yt-fido test this please

@matthewturk
Copy link
Member

yup

@neutrinoceros
Copy link
Member Author

@matthewturk right on, this is now ready !

@matthewturk matthewturk merged commit 41c8259 into yt-project:main Apr 14, 2022
@neutrinoceros neutrinoceros deleted the lower_bound_cyl_pix branch April 14, 2022 13:47
@neutrinoceros
Copy link
Member Author

@meeseeksdev backport to yt-4.0.x

meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Apr 14, 2022
…completely within the data domain in pixelize_cylinder routine
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