-
Notifications
You must be signed in to change notification settings - Fork 276
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
possible bug in Ray objects: calculation of projected lengths dl for non-grid/SPH data #4781
Comments
Hi, and welcome to yt! Thanks for opening your first issue. We have an issue template that helps us to gather relevant information to help diagnosing and fixing the issue. |
Hi! I've read through this in detail, and I believe that your exploration of the problem is spot on. I have one question, which is why the value is 0.30000 for the old value -- do you have a sense if that number falls out of the factor difference? For reference I've also LaTeX'd the first few equations up so I could read them more easily: The test you've applied seems precisely right to me, and given the convergence you're seeing I think your choice of parameters is good. |
Was it ever identified if this same weighting was present in the |
I believe this is up for discussion on Slack right now |
I have identified one potential way to test the existing weighting The potential bug that you have identified only affects particle-based codes, not grid-based codes. So we could take a grid-based snapshot, calculate a ray on a specific trajectory through that volume to measure some field (say, |
Yes, it is up for discussion on slack, but I wasn't sure where this discussion was supposed to be happening, either here or on slack. It seems like this may be a better place for it since it has more permanence than our slack discussions. |
I've brought up a possible issue with the The backend function Thanks for the LaTeXing, by the way, @matthewturk. I've fixed the rest as well; I wasn't aware we could use LaTeX math here. |
The small-particle issue for the SPH |
I like @chummels test idea. One other option I was wondering about in this context is if there are functions/methods/datasets for testing very simple setups for this kind of thing, e.g., a dataset with one, or at least |
The volume integral of 0.300 is only for the cubic kernel; other kernels have differences by other factors, so I suspect there isn't something very deep going on there. |
@nastasha-w so there are a few simple test setups in yt already; take a look at these to see if they are super relevant:
I think I believe I exchanged emails with one or more people where they conducted comparisons of different smoothing kernels and closure applications, which I will try to dig up. (Unfortunately the rest of my day is teaching, so I may have to come back to this at another time.) |
Thanks! I've been looking into those. One issue has already come up: def _generate_container_field(self, field):
# What should we do with `ParticleDataset`?
if isinstance(self.ds, SPHDataset):
return self._generate_container_field_sph(field)
else:
return self._generate_container_field_grid(field) I think that question is coming up here; I can set the ray0._generate_container_field_sph("t")
ray0._generate_container_field_sph("dts") but trying ray0["t"] gives an error:
The comment mentions the Anyway, checking |
@nastasha-w did we ever carry out the test that @chummels suggested in #4781 (comment)? Regardless, I have read through this issue and your derivation and I believe that you are correct. I think it would be good to verify this empirically. @chummels @matthewturk any other comments here? these issues have been lingering for too long and we need to resolve them. |
@jzuhone I did not carry out that test, no. In hindsight, I think the 'scatter' SPH gridding algorithm had an issue in it as well (#4788, #4939), so that might complicate this approach. I ended up going with a test (added to the test suite) where I calculate the kernel integrals with a separate function using numpy and compare that against the |
I believe myself and the NCSA intern I had working with me at the time are the original authors of the code you've worked through. Thanks so much for taking the time to fix and expand this code. My hope was always that somehow who is more of an expert in SPH would find it useful and improve it. |
Bug report
Bug summary
There is a possible bug in the calculation of lengths dl in Ray objects for SPH and other non-grid data. I think the Ray object is inputting (impact parameter / smoothing length) into a cython function where it should be inputting (impact parameter / smoothing length)^2. This would mean lengths dl, and derived quantities like surface densities or column densities are systematically underestimated.
Bug reasoning
As far as I can tell, the under-the-hood code to calculate a resolution element’s projection onto a pencil beam (calculation of dl in a Ray object) comes from the SPLASH paper, which I think is this one: SPLASH paper?.
From that paper, the basic equation (eq. 3) to start from is that in an SPH simulation, a field A(x) is sampled by SPH particles such that
where x is the position (vector), j denotes the particle index, W is the kernel function, and h is the smoothing length. In practice, the kernel only depends on$|x - x_j| / h_j$ , and we can write a dimensionless kernel $U(r)$ , where $r$ is the normalized distance to the particle center, such that (eq. 8)
$W(|x−x_j|, h_j) = U(|x−x_j| / h_j) / h_j^3$ (for 3-dimensional data).
To calculate a column density$N$ , we want to integrate a density field $n(x)$ along some line of sight $l$ . This means
where$x(l)$ is the path through the volume.$x(l) = x_0 + l \hat{x}$ , where $\hat{x}$ is a unit vector describing the direction of the path. Examining an integral for one particle, we can set the zero point $x_0$ where the path is closest to $x_j$ , at impact parameter $b$ , and set $x_j$ as the center of the coordinate system. Then
For the straight line paths of the ray object,
where$r = l / h_j$ .
We then get
This integral$\int U\left( \sqrt{(b/h)^2 + r^2} \right) dr$ depends only on the normalized impact parameter $b/h$ , so in yt, https://github.com/yt-project/yt/blob/main/yt/utilities/lib/pixelization_routines.pyx, line 1024 onward, the class
SPHKernelInterpolationTable
is created, which calculates a grid of these integrals for a given kernel, and then uses linear interpolation to quickly calculate these values for many SPH particles. This class seems to interpolate between squared, normalized impact parametersq2
, e.g. in this trapezoidal integral calculation:No squares/square roots seem to be taken when storing the
q2
values and their integral results in a table, or when the input value ofinterpolate_array(self, np.float64_t[:] q2_vals)
is compared to the table values. Thepixelize_sph_kernel_projection
function, also in https://github.com/yt-project/yt/blob/main/yt/utilities/lib/pixelization_routines.pyx, does input (impact parameter / smoothing length)^2 intointerpolate_array
in its projection routine. However, in the yt Ray object (https://github.com/yt-project/yt/blob/main/yt/data_objects/selection_objects/ray.py), the_generate_container_field_sph
function for the fielddts
(dl
/ total length of the Ray) calls interpolate_array asThese$dl$ values, as used in e.g., Trident to calculate column densities from particle ion densities, are supposed to be the
factors that determine each particle's contribution to the total, so that
It is therefore a pretty serious problem if these factors are not calculated right.
Code test
To check whether these integrals make sense, I did a simple test making use of the volume integral of the kernel. Going back to the initial definition, and doing a volume integral of the density:
where I've changed coordinates to$r' = r / h_j$ . Simplifying,
As long as each particle's kernel is fully contained in the volume, its mass contribution to the total should simply be$m_j$ .
Integrating over the whole volume of a simulation, then
In other words,$\int U(r) dV = 1$ .$dl$ kernel calculation by separating the volume integral into a line integral along parallel lines of sight and a surface integral perpendicular to these:
We can use this to test the
where$r_{\perp}$ is the impact parameter.
I've tested inputting these normalized impact parameters into the table interpolator, and their squared values, to calculate the inner integral, then done the outer integral over these with a simple sum to test the volume integral constraint.
outcome
These volume integrals seem to favor the squared input, like my examination of the code did. I have also checked that the outcome is the same if I use
redges = np.linspace(0., 2., 2000)
, so the issue is not that the normalized kernel has a larger support than I thought.Proposed fix
The fix, if I am right, would be very simple: just replace
dl = itab.interpolate_array(b / hsml) * mass / dens / hsml**2
bydl = itab.interpolate_array((b / hsml)**2) * mass / dens / hsml**2
. However, I wanted to make sure I got this right before messing with the code.Version Information
I installed both python and yt from miniconda, using the default channel.
The text was updated successfully, but these errors were encountered: