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

Alternative notebook to convert WFM data to wavelength #57

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Oct 7, 2024

Supersedes #53.

This is a different approach to computing wavelengths for WFM data.
Instead of runnig the simulation with one opening at a time, finding frame edges (removing outliers), then applying a time offset to each frame, we define a function that connects tof to wavelength.

This is done by plotting wavelength as a function of tof for the neutrons that make it to the detector
Screenshot at 2024-10-07 15-27-35
(this is the same figure as those made by scippneutron's chopper cascade module).

Then, we take the mean wavelength inside each tof bin, and use that as a 1d linear interpolator that computes wavelengths given an array of tofs
Screenshot at 2024-10-07 15-29-37

This is 100 times simpler than the old approach and yields very good results:
Screenshot at 2024-10-07 15-31-18

Copy link

@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.

I think we should consider renaming the tof coordinate in this library to toa to avoid frequently having to explain that actually it refers to the time of arrival and not time of flight.

I wanted a figure that more quantitatively illustrates the errors of the two methods.
To do that I made a figure that shows the probability a computed wavelength has relative error above x, as a function of x. I think it came out quite well. Maybe it is something we can include in the notebook.

grid = sc.geomspace('relative_error', 1e-3, 0.2, 100)

err_wfm = sc.abs(events.flatten(to='event').coords['wavelength'] - flat.coords['wavelength']) / events.flatten(to='event').coords['wavelength']
err_naive = sc.abs(events.flatten(to='event').coords['wavelength'] - naive.coords['wavelength']) / events.flatten(to='event').coords['wavelength']

err_wfm = sc.cumsum(err_wfm.hist(relative_error=grid))
err_naive = sc.cumsum(err_naive.hist(relative_error=grid))

p = pp.plot({'wfm': 1 - err_wfm / sc.max(err_wfm), 'naive': 1 - err_naive / sc.max(err_naive)}, scale={'relative_error': 'log'})
p.ax.set_ylabel('Probability of $rel. err. > x$')
p

error-illustration

@nvaytet
Copy link
Member Author

nvaytet commented Oct 10, 2024

I think we should consider renaming the tof coordinate in this library to toa

I think I agree with this but this would break all the existing notebooks that users have. We should probably keep the old tof coordinate as well for some time period, to avoid too many issues, with a deprecation period?

Shame that the library is called tof... 😬

@nvaytet
Copy link
Member Author

nvaytet commented Oct 10, 2024

In your calculation of err_wfm, I think some of the wavelengths are NaN (in between frames). Is this properly handled in what you did? (I don't know how cumsum handles NaNs)

@jokasimr
Copy link

I think we should consider renaming the tof coordinate in this library to toa

I think I agree with this but this would break all the existing notebooks that users have. We should probably keep the old tof coordinate as well for some time period, to avoid too many issues, with a deprecation period?

Yes that's a problem. But maybe a good opportunity for us to practice doing a proper deprecation cycle.

Shame that the library is called tof... 😬

No I think that's still a fine name!

@jokasimr
Copy link

In your calculation of err_wfm, I think some of the wavelengths are NaN (in between frames). Is this properly handled in what you did? (I don't know how cumsum handles NaNs)

That's handled in .hist. Note that we are histogramming a sc.Variable there, so if the variable is nan it doesn't fit in any bin and is dropped.

@nvaytet
Copy link
Member Author

nvaytet commented Oct 10, 2024

In your calculation of err_wfm, I think some of the wavelengths are NaN (in between frames). Is this properly handled in what you did? (I don't know how cumsum handles NaNs)

That's handled in .hist. Note that we are histogramming a sc.Variable there, so if the variable is nan it doesn't fit in any bin and is dropped.

Even if you look at scipp/scipp#3556 ?

@nvaytet
Copy link
Member Author

nvaytet commented Oct 10, 2024

If I histogram with enough bins, I can see the nans in there (see left edge in plot). They just don't show up in the other plots because they don't contribute much. This would be fixed by fixing scipp.
Screenshot at 2024-10-10 12-25-03

@jokasimr
Copy link

In your calculation of err_wfm, I think some of the wavelengths are NaN (in between frames). Is this properly handled in what you did? (I don't know how cumsum handles NaNs)

That's handled in .hist. Note that we are histogramming a sc.Variable there, so if the variable is nan it doesn't fit in any bin and is dropped.

Even if you look at scipp/scipp#3556 ?

Good to know! I assumed they were dropped

Copy link

@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.

With the toa issue resolved in #59 I'm happy to see this merged

@nvaytet nvaytet merged commit 60ffd1d into main Oct 10, 2024
3 checks passed
@nvaytet nvaytet deleted the wfm-wavelength-conversion branch October 10, 2024 12:12
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

Successfully merging this pull request may close these issues.

2 participants