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: wrong pixelization for "r-normal" projections in spherical coordinates ? #3610

Closed
neutrinoceros opened this issue Oct 19, 2021 · 8 comments · Fixed by #3628
Closed

BUG: wrong pixelization for "r-normal" projections in spherical coordinates ? #3610

neutrinoceros opened this issue Oct 19, 2021 · 8 comments · Fixed by #3628

Comments

@neutrinoceros
Copy link
Member

Bug report

Bug summary
originally reported as #3529

Code for reproduction

import yt

ds = yt.load_sample("bw_spherical_2d", unit_system="code")
p = yt.SlicePlot(ds, "r", "density")
p.save(f"/tmp/test.png")

Actual outcome
test

Expected outcome
Seemingly, working examples of this usage were documented in #1791
but it also looks like the important code here has really not changed much since then (except for formatting), and the PR included tests, so I'm surprised to see it failing now. I'll try to inspect this deeper later.

@neutrinoceros
Copy link
Member Author

Here's another example that doesn't require yt.load_sample or the AMRVAC frontend, that I was able to build from @matthewturk's notebook embedded in #1791 (thank you so much for that Matt !)

It should be pretty clear here that the theta coordinate should go up to 2*PI

import numpy as np
import yt

shape = (72,  721, 1152)
prng = np.random.RandomState(0)
data = {("gas", "density"): prng.random_sample(shape)}
bbox = np.array([[1, 72], [0, 2.0*np.pi], [0, np.pi] ])

ds = yt.load_uniform_grid(
    data,
    shape,
    1.0,
    geometry=("spherical", ('r', 'theta', 'phi')),
    bbox=bbox
)

p = yt.SlicePlot(ds, "r", "density")
p.save(f"/tmp/test.png")

test

@neutrinoceros
Copy link
Member Author

Using this patch

diff --git a/yt/utilities/lib/pixelization_routines.pyx b/yt/utilities/lib/pixelization_routines.pyx
index 72803050c..ab7e896c7 100644
--- a/yt/utilities/lib/pixelization_routines.pyx
+++ b/yt/utilities/lib/pixelization_routines.pyx
@@ -687,10 +687,10 @@ def pixelize_aitoff(np.float64_t[:] theta,
                 phi0 = math.asin(z*y)
                 # Now we just need to figure out which pixel contributes.
                 # We do not have a fast search.
-                if not (theta_p - dtheta_p <= theta0 <= theta_p + dtheta_p):
-                    continue
-                if not (phi_p - dphi_p <= phi0 <= phi_p + dphi_p):
-                    continue
+                #if not (theta_p - dtheta_p <= theta0 <= theta_p + dtheta_p):
+                #    continue
+                #if not (phi_p - dphi_p <= phi0 <= phi_p + dphi_p):
+                #    continue
                 img[i, j] = field[fi]
     return img

recompiling and running the example above gives
test

so it seems clear that the pixels are correctly selected, but apparently half of them are not correctly initialised

@neutrinoceros
Copy link
Member Author

My example above is broken on the user side... theta is the angle that varies over a PI-wide interval, while phi varies over 2PI. The code dealing with this in yt is known to be inconsistent in naming these (#1796), but it should be ok on the user side. Here's a corrected version of my test

import numpy as np
import yt

shape = (72,  721, 1152)
prng = np.random.RandomState(0)
data = {("gas", "density"): prng.random_sample(shape)}
bbox = np.array([[1, 72], [0, np.pi], [0, 2*np.pi]])

ds = yt.load_uniform_grid(
    data,
    shape,
    1.0,
    geometry=("spherical", ('r', 'theta', 'phi')),
    bbox=bbox
)

p = yt.SlicePlot(ds, "r", "density")
p.save(f"/tmp/test1.png")


# transposition
bbox = np.array([[1, 72], [0, 2*np.pi], [0, np.pi]])

ds = yt.load_uniform_grid(
    data,
    shape,
    1.0,
    geometry=("spherical", ('r', 'phi', 'theta')),
    bbox=bbox
)

p.save(f"/tmp/test2.png")

both example produce this image
test1

which I think is incorrect ? I'd except x/y axes to be flipped, now let's try fix that.

@matthewturk
Copy link
Member

A few minor things -- before you get too far in, can you check what happens if you use the cartopy projections?

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Oct 19, 2021

I'd love to, how do I do that ?

@neutrinoceros
Copy link
Member Author

(I might be unable to actually, cartopy is notoriously hard to install outside of conda land)

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Oct 21, 2021

yet another way this is broken:

import yt

ds = yt.load_sample("KeplerianDisk")
p = yt.SlicePlot(ds, "r", "density")
p.save(f"/tmp/disk_slice_r.png")
unroll !

disk_slice_r

likely a crucial difference with the previous example is that this data set has

>>> ds.coordinates.axis_order
('r', 'theta', 'phi')

while the previous one had

>>> ds.coordinates.axis_order
('r', 'phi', 'theta')

@neutrinoceros neutrinoceros removed this from the 4.1.0 milestone Oct 21, 2021
@neutrinoceros
Copy link
Member Author

I think I'll need to adress #1796 thoroughly before I can fix this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants