Use xl, yl, zl for origin of diffraction calculation in PBP indexer #342
Conversation
| eta_local = eta[m] | ||
|
|
||
| # mask g-vectors to [m & mr] | ||
| gv_local_mask = gv[local_mask[m], :] |
There was a problem hiding this comment.
The comment suggests this should be: gv[ m & mr , : ] or gv[ local_mask, : ]. Not sure why there is an [m] on the mask?
There was a problem hiding this comment.
My thought process went like this:
We want to calculate the g-vectors for all the peaks at this pixel (mask m).
However, we want to index only the g-vectors that are also in isel (mask mr).
We compute gv (and gx, gy, gz) from peaks[m] (the dtyi, omega mask) so len(gv) = m.sum() != len(sinomega).
However mr has the wrong length to directly mask gv.
If we combine local_mask = m & mr first, then we can mask local_mask by m to get the mr mask for only the m-masked values, so the masks have the right lengths.
Therefore gv[local_mask[m], :] should be correct.
There was a problem hiding this comment.
If you want the ring mask at this pixel I would expect to read that as mr[ local_mask ] ?
There was a problem hiding this comment.
The way I see it is that we are creating a (ring and pixel) mask for all the peaks using local_mask = m & mr, then selecting a (pixel) subset of that with local_mask[m] which then has the same length to index gv (which was also created from peaks[m]).
There was a problem hiding this comment.
Perhaps the problem is just unclear variable naming?
m: Mask on all peaks to select those that come from the pixel
mr: Mask on all peaks to select those on the ring (isel)
local_mask: Mask on all peaks to select those on the ring AND those from the pixel
There was a problem hiding this comment.
It seems like it can also be gv[ mr[m] ] with a comment like # selection: mr = active on rings, [m] at this point in space.
The thing you have is:
gv[ ( m & mr )[m] ]
While I see this is mathematically equivalent, having the m in there twice meant I could not figure out what you were doing.
Feel free to merge and I can always change it later.
| # ignore Numba dot product performance warnings? | ||
| import warnings | ||
|
|
||
| warnings.simplefilter('ignore', category=numba.core.errors.NumbaPerformanceWarning) |
There was a problem hiding this comment.
Rather than suppressing the message, we should eventually fix the actual problem here
| # compute g-vectors from xyz | ||
| ImageD11.cImageD11.compute_gv(xyz_local, | ||
| omega, | ||
| parglobal.get('omegasign'), |
There was a problem hiding this comment.
parglobal is not defined inside this function
There was a problem hiding this comment.
As parglobal is globally defined, I don't think it needs to be defined inside this function? It seems to run on my machine, anyway...
| @@ -1,6 +1,10 @@ | |||
| import os | |||
|
|
|||
| import numba | |||
| # ignore Numba dot product performance warnings? | |||
| import warnings | |||
| warnings.simplefilter('ignore', category=numba.core.errors.NumbaPerformanceWarning) | |||
There was a problem hiding this comment.
I would like to understand why it gives the warning
There was a problem hiding this comment.
We see lots of console spam for the np.dot functions being run on non-contiguous arrays - mainly for the hkl calculations - I think this is unavoidable?
There was a problem hiding this comment.
If we suppress the warning we should create an issue to come back to it sometime in the future. Like this I am afraid we lose all of the feedback on numba compilation for anything importing our code...
This might be tricky - we're now directly computing the g-vectors from |
|
Dstar is the length of gv. I guess the function can be tested outside of the call here. |
|
Just tested this against the previous version and it should be equivalent - should be good to merge |
@jonwright can you please double-check the maths behind this? I believe I am correctly modifying the distance but it'd be good to double-check: the new function
get_local_gvdoes this for the indexer.