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

Don't allow edges to extend beyond domain for SPH region sources. #2644

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

matthewturk
Copy link
Member

This showed up when generating the gallery using the TipsyGalaxy dataset. What would happen is that if you set the width to (1.0, 'unitary') it would extend the plot beyond the edge of the domain, which would then create a region that went beyond the edge of the domain, and then break because the dataset wasn't periodic. This prevents that from happening.

@matthewturk
Copy link
Member Author

I had to update this (I am sorry!) because I realized that when the data is periodic, we want the region to go as it was, over the boundaries.

@cphyc @Xarthisius @munkm another quick look?

Copy link
Member

@munkm munkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the updated logic looks good! I added a comment that's not super important for merging, but might help future code investigators.

@neutrinoceros
Copy link
Member

Sidenote, I can't wait to test this against my own (amrvac) datasets. I've been using workarounds to avoid this issue, and never took the time to report or inspect it. Thanks a lot !

Copy link
Member

@chummels chummels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. It would be cool to see what different behavior this produces for the TipsyGalaxy answer test projections, which probably would address the concerns I brought up in my review of the answer test results for it.

@matthewturk
Copy link
Member Author

matthewturk commented Jun 18, 2020 via email

@munkm munkm merged commit 59f9432 into yt-project:yt-4.0 Jun 19, 2020
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.

6 participants