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: fix pixelize_cylinder #3782

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Feb 6, 2022

PR Summary

Fix #3781
I found that the actual problem was that the jumps in r and theta were constrained only by dx (horizontal pixel size) when they needed to be constained by dy too.

Using a variation of the script from the issue to show the change

import numpy as np
import yt

shape = (32, 64, 1)
a = np.linspace(0, 1, 64)
b = np.ones((32, 64))
c = np.reshape(a*b, shape)
data = {("gas", "density"): c}

theta_min = np.pi / 2 - 0.1

ds = yt.load_uniform_grid(
    data,
    shape,
    bbox=np.array([[0.5, 5.0], [theta_min, theta_min+0.1], [-0.1, +0.1]]),
    geometry="spherical",
)

yt.SlicePlot(ds, "phi", "density").save("/tmp/")

on the main branch

main

this branch
patch

@neutrinoceros
Copy link
Member Author

Switching to draft until I have time to go dig into failing tests.

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Feb 7, 2022

Turns out the error is unrelated, and caused by a simple deprecation warning from SciPy 1.8
I'm pushing the fix here to make CI go green but I'll also make a separate PR to fix the problem for everyone
edit: this is #3783

@neutrinoceros neutrinoceros force-pushed the fix_cyl_pix branch 2 times, most recently from 9e7e166 to b33a8e7 Compare February 7, 2022 09:13
@neutrinoceros
Copy link
Member Author

In fact I'll leave this as a draft until I can test that my optimisations do not break other use cases

@neutrinoceros
Copy link
Member Author

  • failing tests on Jenkins may just need to be bumped (to be checked manually)
  • I think my implementation can be optimised still: radial and angular increments in the loop are currently not dependent on theta, so they are de facto smaller than a pixel in most cases. This could be accelerated with a bit of geometry.

@matthewturk
Copy link
Member

I think this is the correct fix. Thank you!

@neutrinoceros neutrinoceros marked this pull request as ready for review February 7, 2022 14:01
@neutrinoceros
Copy link
Member Author

Got it back to a safe state with no overly eager optimisation after I found that I was reintroducing similar bugs. This should now be mergeable

@matthewturk
Copy link
Member

It wasn't obvious to me that any optimizations were necessary.

@neutrinoceros
Copy link
Member Author

Oh I just felt bad about making this routine a little slower since it's now doing more jumps. But I guess I shouldn't use a broken implementation as a baseline !

@matthewturk matthewturk merged commit b359421 into yt-project:main Feb 7, 2022
@neutrinoceros neutrinoceros deleted the fix_cyl_pix branch February 7, 2022 14:42
neutrinoceros pushed a commit to neutrinoceros/yt that referenced this pull request Feb 7, 2022
@neutrinoceros neutrinoceros added this to the 4.0.3 milestone Apr 19, 2022
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.

BUG: Moirés in cylindrical pixelization
2 participants