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

pbp indexer - y0 not added in grid and units for diffty not matching distance #366

Closed
jonwright opened this issue Dec 13, 2024 · 11 comments
Closed
Assignees

Comments

@jonwright
Copy link
Member

Around line 2000 in point_by_point:
"""
nlo = np.floor((self.ybincens.min()) / self.ystep).astype(int)
nhi = np.ceil((self.ybincens.max()) / self.ystep).astype(int)

    rng = range(nlo, nhi + 1, gridstep)

"""
It needs to subtract y0 if there was one.

Also noted: the distance correction in get_local_gv assumes dty and distance have the same units. This is harder to fix automagically unless everything comes with units.

@jonwright
Copy link
Member Author

Also the indexing loglevel does not work when changed. Something about the multiprocessing pool and global state perhaps. No idea how to fix that.

@jadball
Copy link
Contributor

jadball commented Dec 13, 2024

For the first issue: here's the commit to blame: 32dfd94

It seems I was correcting a y0 bug for a dataset with a big y0 by modifying the code in two places - I remember things definitely got better after the bug fix, but I suspect the second fix wasn't needed. To me this raises deeper questions about how the geometry.dty_to_dtyi function works/should work. TODO after Christmas for sure!

@jadball
Copy link
Contributor

jadball commented Dec 13, 2024

I think the underlying problem is that we assume that a perfectly aligned diffractometer would have a y0 value of 0?

@jadball
Copy link
Contributor

jadball commented Jan 8, 2025

The simplest fix for this (without possibly introducing more bugs by modifying geometry.py) would be to have an option to shift self.dty in the DataSet class when the motor values are read/imported, to account for where the rotation axis should be in the lab frame assuming perfect alignment (~14 for the TDXRD station, 0 for the NSCOPE station). @jonwright what do you think? The alternative is I think some rather drastic additions to geometry.py to introduce a new static reference frame centered on the diffractometer.

@jonwright
Copy link
Member Author

I didn't understand why the 14 is different to y0 yet? I will try to go through the code soon.

Just adding some known offset to the silicon testcase should give something small to look at numbers and debug...

@jonwright
Copy link
Member Author

Is it just:

Sinogram scan step space != Recon step space

?

@jadball jadball self-assigned this Jan 13, 2025
@jadball
Copy link
Contributor

jadball commented Jan 15, 2025

Is it just:

Sinogram scan step space != Recon step space

?

@jonwright you're right.
Continuous (dty, omega) space should have its own discretisation.
By convention we use (ystep, ostep).
As the 'centre' of the dty scan can be arbitrary (e.g NSCOPE vs 3DXRD), I propose that dtyi = 0 happens at the lowest-value dty scan (e.g ds.ymin) such that:

def dty_to_dtyi(dty, ystep, ymin):
    """
    We take continuous dty space and discretise it
    We do this by counting the number of ysteps away from ymin
    ymin should probably be ds.ymin
    This should be equivalent to determining the closest bin in ds.ybincens
    But this is neater as it accounts for values outside the ds.ybincens range

    dtyi = (dty - ymin) / ystep
    """
    dtyi = np.round((dty - ymin) / ystep).astype(int)
    return dtyi

Merge coming soon for this bug fix, including standardised ways of producing grids in step space which is I think where this issue appeared

@jonwright
Copy link
Member Author

Ymin is probably dataset.ybincens[0] with dataset.ystep for the step. That should work for now, but for a sinogram that is not on a regular grid we might have to come back to this.

Thanks for fixing this!

@jadball
Copy link
Contributor

jadball commented Jan 15, 2025

@jonwright looking at point_by_point.py, we often need a method to construct a grid in step space (si, sj) to reconstruct on.
What do you think of the proposal below?
It means that even for an unsymmetric scan we get symmetric reconstruction grids:

def step_grid_from_ybincens(ybincens, step_size, gridstep, y0):
    """
    Produce a grid in (si, sj) step space to reconstruct on.
    Constructs a symmetric grid with the origin on y0
    Therefore the step space is truly aligned to the rotation axis.
    The size of the grid is determined by the extremes of ybincens.
    The grid will always be symmetric such that (0, 0) is the centre of an image formed from the grid
    :param ybincens: An evenly sampled dty space
    :type ybincens: np.ndarray
    :param step_size: the step size of the grid (same units as ybincens and y0)
    :type step_size: float
    :param gridstep: the sampling resolution of the grid (e.g 2 = [0, 2, 4, 6, ...])
    :type gridstep: int
    :return: (si, sj) for si in ints for sj in ints
    :rtype: tuple(int, int)
    """
    # centre y range on y0
    y_relative = ybincens - y0
    # find minimum and maximum values of y
    y_min = y_relative.min()
    y_max = y_relative.max()
    # work out which is larger, so our grid is always symmetric
    # i.e never go from (-65, 65) to (100, 100)
    # always go from (-100, -100) to (100, 100)
    y_largest = np.max(np.abs([y_min, y_max]))
    y_min = -y_largest
    y_max = y_largest
    y_min_int = np.floor(y_min/step_size).astype(int)
    y_max_int = np.ceil(y_max/step_size).astype(int)
    ints = range(y_min_int, y_max_int + 1, gridstep)
    step_points = [(si, sj) for si in ints for sj in ints]
    return step_points

@jadball
Copy link
Contributor

jadball commented Jan 16, 2025

^ per my previous comment, this tweak is already paying dividends when we have a significant (y0-ybeam) error:

Image

On the right image, the step grid we have reconstructed on is automatically large enough to account for the effects of the scan not being well centered to the beam, because we are considering the true y0 value when constructing our step grid.

@jadball jadball mentioned this issue Jan 16, 2025
@jadball
Copy link
Contributor

jadball commented Jan 18, 2025

Resolved by #379 :)

@jadball jadball closed this as completed Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants