release gil in pixelize_cylinder

[chrishavlin]

release the gil!

This surprisingly seemed to work with minimal effort... took the pixelization time from 53s to 27s for one example (see this slack thread).

[neutrinoceros]

Releasing the GIL is likely a good idea here, but I also suspect that the abysmal performance in this routine may be fixed independently (possibly by reverting an earlier patch from mine that I'll dig up for in my future review). Any way, I'll try to review in details in the coming days. Thanks !

[chrishavlin]

if you figure out a separate fix I'm happy to go with it and close this one!

[chrishavlin]

oh, especially since image tests seem to be failing and it was not as simple as i hoped :)

[chrishavlin]

ok, i think i see the issue but won't have time to fix it for a while... so will hold off in case you devise a separate performance fix.

[neutrinoceros]

oh wow these diffs are catastrophic. I'll try to tackle the performance issue from another angle and I'll move this to draft if you don't mind.

[matthewturk]

These lines:

                     ptbounds[0] = ptbounds[0] % (2*np.pi)
                     ptbounds[1] = ptbounds[1] % (2*np.pi)

are using Python math operations inside a tight loop. What if we used a constant for PI instead of np.pi, which I believe will do a python-level lookup?

[cphyc]

are using Python math operations inside a tight loop. What if we used a constant for PI instead of np.pi, which I believe will do a python-level lookup?

With the latest version, they are actually expanded to

(__pyx_v_ptbounds[0]) = fmod((__pyx_v_ptbounds[0]), (2.0 * __pyx_v_npPI));

so there might be a small speedup to be found by precomputing 2π ahead of time, but I'm not sure this is super important.

[cphyc]

Is this a case where we'd want to use

for i in prange(radius.shape[0], nogil=True):
   pass

[chrishavlin]

sure! I think prange should work here and could give an extra performance boost. but i'll wait until the rest of the PR is actually working before I change it.

[chrishavlin]

ok well using prange might be a bit more work than I want to put in right now -- lots of compilation errors along the lines of :

      yt/utilities/lib/pixelization_routines.pyx:635:20: Cannot read reduction variable in loop body
      
      Error compiling Cython file:
      ------------------------------------------------------------
      ...
                  while r_i < r0 + dr_i:
                      if rmax <= r_i:
                          r_i += r_inc
                          continue
                      y = r_i * costheta
                      x = r_i * sintheta
                          ^
      ------------------------------------------------------------
      
      yt/utilities/lib/pixelization_routines.pyx:636:20: Cannot read reduction variable in loop body

[matthewturk]

are using Python math operations inside a tight loop. What if we used a constant for PI instead of np.pi, which I believe will do a python-level lookup?

With the latest version, they are actually expanded to

(__pyx_v_ptbounds[0]) = fmod((__pyx_v_ptbounds[0]), (2.0 * __pyx_v_npPI));

so there might be a small speedup to be found by precomputing 2π ahead of time, but I'm not sure this is super important.

Huh. I guess I wasn't on the latest version! For me it did a ton of unpacking and python type coercion. I guess that's one of the advantages of upgrading, eh?

Are our performance tests being run on the latest version? (i.e., do you see the slowness, or is what I am referring to showing up in our benchmarks)

[chrishavlin]

still expect the image diffs to fail -- i'll look later today and if it's a quick fix (which i think it is) I'll put it in and do some performance tests.

[neutrinoceros]

here's the direct way to "import" the constant from numpy as the C level

cdef extern from "numpy/npy_math.h":
    double NPY_PI

(taken from https:/yt-project/yt/pull/4948/files#r1692841029)

[neutrinoceros]

(NPY_2_PI is also available)

[chrishavlin]

hah, will import NPY_2_PI as well in a minute after tests run this round.... i think this one will pass...

[chrishavlin]

oh wait, NPY_2_PI is 2/pi, not 2*pi ?

[neutrinoceros]

aaaah, missed that, sorry

[chrishavlin]

so I'm not sure why I needed to add the check for negative numbers here, but after switching from ptbounds[1] % (2*np.pi) to ptbounds[1] % twoPI, the result returns numbers in the range (-pi,pi).

[neutrinoceros]

I think we need to understand this bit, and possibly come up with a way to perform the computation that doesn't involve branching; hard-wiring a somewhat expensive calculation generally hurts performance less than ruining branch prediction.
I'm writing this comment early in my review but I'll try to come up with more insight.

[neutrinoceros]

I'm taking a bet that the reason why you're seeing a change in behavior is that the function is decorated with @cython.cdivision(True): on main the Python % is used, while with twoPI you're getting a C operator instead.

http://docs.cython.org/en/latest/src/tutorial/caveats.html

[neutrinoceros]

can you try this patch

diff --git a/yt/utilities/lib/pixelization_routines.pyx b/yt/utilities/lib/pixelization_routines.pyx
index 194ce7af6..6728375d0 100644
--- a/yt/utilities/lib/pixelization_routines.pyx
+++ b/yt/utilities/lib/pixelization_routines.pyx
@@ -664,12 +664,8 @@ def pixelize_cylinder(np.float64_t[:,:] buff,
                             ptbounds[1] = fmax(ptbounds[1], corners[i1])
 
                         # shift to a [0, PI] interval
-                        ptbounds[0] = ptbounds[0] % twoPI
-                        if ptbounds[0] < 0:
-                            ptbounds[0] += NPY_PI
-                        ptbounds[1] = ptbounds[1] % twoPI
-                        if ptbounds[1] < 0:
-                            ptbounds[1] += NPY_PI
+                        ptbounds[0] = (ptbounds[0]+twoPI) % twoPI
+                        ptbounds[1] = (ptbounds[1]+twoPI) % twoPI
 
                         if prbounds[0] >= rmin and prbounds[1] <= rmax and \
                            ptbounds[0] >= tmin and ptbounds[1] <= tmax:

the result should be the same in all cases, but it avoids branching, so I expect it to be faster.
If this works, we'll want to add a comment for why it's done this way.

[chrishavlin]

Here's a performance comparison between this branch and main that compares the scaling with increasing number of cells in r and theta.

Full code for the test case is over in this repo. The test dataset is a 2d cylindrical dataset built with:

def get_2d_ds(n_r, n_theta):

    shp = (n_r,n_theta,1)
    bbox = np.array([[0, 1], [0, np.pi*2],[-.5, .5]])
    data = {
         'density': rs.random(shp)
            }
    ds = yt.load_uniform_grid(data,
                              shp,
                              bbox=bbox,
                              geometry='cylindrical',
                              axis_order=('r', 'theta', 'z'))
    return ds

And I'm timing a SlicePlot here:

def plot_ds(ds):
    slc = yt.SlicePlot(ds, 'z', ('stream', 'density'))
    slc.render()

The first and second column in the following graph are the elapsed time in seconds averaged over 10 iterations for the main branch and this PR's branch. Final column is the percent speedup.

Average speedup over all resolutions is about 33%.

[matthewturk]

I like the comparison. I immediately thought, "what about in AMR" then realized this is likely a better general purpose diagnostic.

[neutrinoceros]

I looked for most recent patches to this routine from ~3 years ago when I was focused on correctness and possibly neglecting performance. Here's what I found, in case that's relevant here

• BUG: fix pixelize_cylinder #3782
• BUG: fix a bug in pixelize_cylinder where buffer shape was read backwards #3795
• BUG: don't colorize pixels that are not completely within the data domain in pixelize_cylinder routine #3818

#3782 and #3818 may have impacted performance, so it might be worth trying to revert them and see if the performance changes measurably.
If #3818 is costly, it may be argued that it should be reverted, as it really comes down to an arbitrary choice between two equally valid approaches, with converging results in the limit of infinite resolution.
#3782 seems actually more likely to have a negative impact, but it's a lot less trivial to decide what to do if so. In any case I think it's worth investigating now, and if performance reaches an acceptable level with this PR, maybe we don't need to do anything more. I'd just like it to be an informed decision.