Skip to content

Essreduce + Essimaging: tofectomy#65

Open
nvaytet wants to merge 27 commits intomainfrom
essreduce-tofectomy
Open

Essreduce + Essimaging: tofectomy#65
nvaytet wants to merge 27 commits intomainfrom
essreduce-tofectomy

Conversation

@nvaytet
Copy link
Member

@nvaytet nvaytet commented Feb 25, 2026

Replacing time-of-flight with wavelength in the GenericTofWorkflow, which is now called GenericUnwrapWorkflow.

Note: once this is merged, every package using the GenericTofWorkflow not in the monorepo will break...

@nvaytet nvaytet added the essreduce Issues for essreduce. label Feb 25, 2026
@nvaytet nvaytet changed the title Essreduce: tofectomy Essreduce + Essimaging: tofectomy Mar 9, 2026
@nvaytet nvaytet marked this pull request as ready for review March 10, 2026 07:15
@github-project-automation github-project-automation bot moved this to In progress in Development Board Mar 10, 2026
@nvaytet nvaytet moved this from In progress to Selected in Development Board Mar 10, 2026
Copy link
Contributor

@jokasimr jokasimr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the name, what about WavelengthWorkflow or EstimateWavelengthWorkflow?


@dataclass
class TofLookupTable:
class LookupTable:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about WavelenghtLookupTable? It's a bit more specific.

I'm sure the technique packages will have other lookup tables so we will have to rename this type in the technique packages anyway. Is it easier to directly give it a specific name here instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking a bit more, LookupTable is probably better in this module. We can rename it in the instrument packages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that when you take the module name into account, unwrap.Lookuptable becomes specific enough.


installation
tof/index
unwrap/index
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to name the module wavelength instead of unwrap?

I get that wavelength is such a common variable name, that there will likely be clashes if the module is imported like from ess.reduce import wavelength but I think it is still better than unwrap because that term is a bit overloaded.

unit=time_unit, copy=False
)
tofs = distance / simulation.speed
# tofs = distance / simulation.speed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# tofs = distance / simulation.speed

@jokasimr
Copy link
Contributor

jokasimr commented Mar 11, 2026

There's one thing I've been thinking about related to this change, and it is that there might be a bit higher interpolation error when we interpolate wavelength instead of time-of-flight.

$$ \lambda = \frac{t - t_0(t, L)}{L} $$

while

$$ t_{of} = t - t_0(t, L), $$

assuming $t_0(t, L)$ is an affine function, or slowly varying relative to grid density, linear interpolation in $t$ and $L$ will represent $t_{tof}$ exactly.
But linear interpolation does not represent $\lambda$ exactly even if $t_0$ is slowly varying or affine, so the interpolation error is probably larger when we interpolate $\lambda$ than when we interpolate $t_{of}$.

How to fix this?

If we want to fix this we don't need to change the interfaces or reintroduce tof in the packages.
We just need to pass $t_0$ (or $t_{of}$) instead of wavelength to the interpolation implementation inside WavelengthInterpolator, and then internally in WavelengthInterpolator.__call__ compute wavelength from the interpolated t0.

@jokasimr
Copy link
Contributor

It's very pleasant to review in the new monorepo :) And I like the diff!

@nvaytet
Copy link
Member Author

nvaytet commented Mar 11, 2026

There's one thing I've been thinking about related to this change, and it is that there might be a bit higher interpolation error when we interpolate wavelength instead of time-of-flight.

λ = t − t 0 ( t , L ) L

while

t o f = t − t 0 ( t , L ) ,

assuming t 0 ( t , L ) is an affine function, or slowly varying relative to grid density, linear interpolation in t and L will represent t t o f exactly. But linear interpolation does not represent λ exactly even if t 0 is slowly varying or affine, so the interpolation error is probably larger when we interpolate λ than when we interpolate t o f .

How to fix this?

If we want to fix this we don't need to change the interfaces or reintroduce tof in the packages. We just need to pass t 0 (or t o f ) instead of wavelength to the interpolation implementation inside WavelengthInterpolator, and then internally in WavelengthInterpolator.__call__ compute wavelength from the interpolated t0.

To get the wavelength, we are using the raw wavelengths given by the tof simulation and averaging and interpolating using them directly.
I'm not sure I understand why they are different, and why you have the extra L factor in the first equation? But maybe talking in-person is easier?

@jokasimr
Copy link
Contributor

jokasimr commented Mar 11, 2026

To get the wavelength, we are using the raw wavelengths given by the tof simulation and averaging and interpolating using them directly.

Yeah I know. Maybe talking in person is easier.

The point is, assuming $t_0(t, L)$ is piecewise affine (or almost constant/slowly varying):

  • $t_{of}(t, L)$ is also piecewise affine. This means we can interpolate it with good accuracy using linear interpolation.
  • $\lambda(t, L)$ is not an affine function of $t$ and $L$. This means there will be an interpolation error that is independent of how well $t_0$ can be represented on the interpolation grid.

Here is an illustration explaining the problem:

Figure_1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

essreduce Issues for essreduce.

Projects

Status: Selected

Development

Successfully merging this pull request may close these issues.

2 participants