-
Notifications
You must be signed in to change notification settings - Fork 280
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
yt 4.0 adding arbitrary_grid to interpolate SPH data #1814
Conversation
…identifying any neighbouring pixels? will attempt to debug later
…se) always debug cython code with bounds checking on
…kernel_arbitrary_grid
…ph arbitrary_grid
…rbitary grid class. Appears to be almost working -- other than returning an array of zeros. will debug
…pears to be working
…-arbitrary-grid pulling remote changes
buff[xi, yi, zi] += coeff * kernel_func(qxy) | ||
|
||
if use_norm: | ||
# now we can calculate the normalized image buffer we want to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd just say buffer
since this is a 3D grid, not an image.
if(is_sph_field): | ||
self._fill_sph_particles(fields) | ||
else: | ||
self._fill_particles(part) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's work through this together during our call tomorrow, I'm a little confused about your concern in the PR description and I think it might be easier to chat about it over a telecon.
pz = chunk[(ptype,'particle_position_z')].in_units('cm') | ||
hsml = chunk[(ptype,'smoothing_length')].in_units('cm')*10.0 | ||
mass = chunk[(ptype,'particle_mass')].in_units('g') | ||
dens = chunk[(ptype,'density')].in_units('g/cm**3') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably be doing this in code units. both here and in the image pixelizers. it doesn't really matter which unit system we choose as long as we stick to a single system, but code units is probably the most natural choice.
bounds[4] = self.left_edge[2] | ||
bounds[1] = self.right_edge[0] | ||
bounds[3] = self.right_edge[1] | ||
bounds[5] = self.right_edge[2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these always in 'cm'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it even matter? it might not if the cython function ignores units.
for field in fields: | ||
dest = np.zeros(self.ActiveDimensions, dtype="float64") | ||
|
||
for chunk in self._data_source.chunks(fields, "io"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should pass in [field]
here?
bounds[5] = self.right_edge[2] | ||
|
||
pixelize_sph_kernel_arbitrary_grid(dest,px,py,pz,hsml,mass, | ||
dens,mass,bounds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, did you forget to pass in the field values for the field we're smoothing onto the grid?
@@ -1154,6 +1154,130 @@ def pixelize_sph_kernel_slice( | |||
continue | |||
buff[xi, yi] += buff_num[xi, yi] / buff_denom[xi, yi] | |||
|
|||
@cython.initializedcheck(True) | |||
@cython.boundscheck(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be turned off when we're sure everything is correct
for j in prange(0, posx.shape[0]): | ||
if j % 1000 == 0: | ||
with gil: | ||
PyErr_CheckSignals() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd like to see a progress bar here as well
w_j = pmass[j] / pdens[j] / hsml[j]**3 | ||
coeff = w_j * quantity_to_smooth[j] | ||
|
||
# Now we know which pixels to deposit onto for this particle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
voxels, not pixels
Not at all :) In general you don’t need to ask permission to use code I share publicly, just assume the answer is yes. |
… work with derived fields as advised by Nathan
…utput for the field being interpolated
@ngoldbaum Just tried your script on |
Holy cow--that VR on Gadget data is awesome! |
update:
There was certainly a bug, I'd missed a sqrt which could have been missing particles. I have now fixed this. There also seems to be a difference in the orientation of the return arrays from slice and arbitrary grid. Maybe slice has a transpose I missed or something. If I modify your script to this import yt
import numpy as np
from yt.units import kpc
ds = yt.load('GadgetDiskGalaxy/snapshot_200.hdf5')
_, c = ds.find_max(('gas', 'density'))
width = 50 * kpc
buff_size = 256
field = ('gas', 'density')
# buffer from arbitrary grid
ag = ds.arbitrary_grid(c - width / 2,
c + width / 2,
[buff_size]*2 + [1])
buff_ag = ag[field][:, :, 0].to('g*cm**-3').d
buff_ag = buff_ag.T
# buffer from slice
p = yt.SlicePlot(ds, 'z', field, center=c, width=width)
p.set_buff_size(buff_size)
buff_slc = p.frb.data[field].to('g*cm**-3').d
# comparison plot
from matplotlib.colors import LogNorm
import matplotlib.pyplot as plt
plt.figure(figsize=(10, 4))
plt.subplot(1, 2, 1)
plt.imshow(buff_ag, norm=LogNorm())
plt.colorbar()
plt.title('arbitrary grid')
plt.subplot(1, 2, 2)
plt.imshow(buff_slc, norm=LogNorm())
plt.colorbar()
plt.title('slice')
plt.tight_layout()
plt.savefig('ag_slc_cmp.png') but I don't know why values are off. I think the following line is causing the strange results in the grid, I also think we can avoid the sqrt I had to add by saying |
this is amazing stuff! well done! |
@qobilidop I suspect if you plot that on the same colorbar scale the differences will appear smaller. |
Do you understand why there are zones with no data in the arbitrary grid but not in the slice? |
no I don't, I'm going to look into that now |
It looks like we aren't passing in as many particles in the arbitrary_grid call as we are in the slice call. Any reason why this might be? Every chunk passes more particles to pixelize_slice and than pixelize_arbitrary ... |
Hmm can you share the test script you used to check? Just the one Bili shared upthread? |
I'm using this script (I think its the same as upthread) import yt
import numpy as np
from yt.units import kpc
ds = yt.load('GadgetDiskGalaxy/snapshot_200.hdf5')
_, c = ds.find_max(('gas', 'density'))
width = 50 * kpc
buff_size = 256
field = ('gas', 'density')
# buffer from arbitrary grid
ag = ds.arbitrary_grid(c - width / 2,
c + width / 2,
[buff_size]*2 + [1])
buff_ag = ag[field][:, :, 0].to('g*cm**-3').d
buff_ag = buff_ag.T
# buffer from slice
p = yt.SlicePlot(ds, 'z', field, center=c, width=width)
p.set_buff_size(buff_size)
buff_slc = p.frb.data[field].to('g*cm**-3').d
# comparison plot
from matplotlib.colors import LogNorm
import matplotlib.pyplot as plt
plt.figure(figsize=(10, 4))
plt.subplot(1, 2, 1)
plt.imshow(buff_ag, norm=LogNorm(), vmin=10**(-29), vmax=10**(-22))
plt.colorbar()
plt.title('arbitrary grid')
plt.subplot(1, 2, 2)
plt.imshow(buff_slc, norm=LogNorm(), vmin=10**(-29), vmax=10**(-22))
plt.colorbar()
plt.title('slice')
plt.tight_layout()
plt.savefig('ag_slc_cmp.png') with the extra prints I've just committed. You can see that a differing number of particles is passed. I presume something is going on with the region/slice code? |
# yt's kernel functions use a different convention than | ||
# the SPLASH paper (following Gadget-2), and qxy is | ||
# twice the value of q used in the SPLASH paper | ||
qxy = 2.0 * math.sqrt(posx_diff + posy_diff + posz_diff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just call this q
here and in the other SPH pixelizers. It's called q
in the paper, so it makes it a little easier to compare with the paper. In addition, here z is included as well, so qxy
is a bit of a misnomer.
I'd also add in the comment above to see equation 5 in the SPLASH paper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also should be one half the value in the SPLASH paper, and the factor of 2.0 should be deleted. See the patch I sent you over slack.
Just need to check this still passes my test... will wait for the results |
Great job on seeing this one through Ash! 🎆 💯 🥇 🎉 |
PR Summary
PR to add a new feature to interpolate SPH fields onto an arbitrary grid.
arbitrary_grid
with the slice plotAn example use case of this new wiring is as follows,
PR Checklist