Conversation
github-actions
Bot
changed the title
| providers = ( | ||
| wavelength_range_from_chopper_frames, | ||
| dspacing_bins_from_wavelength_and_two_theta, | ||
| ) |
There was a problem hiding this comment.
I don't think we can simply add all these providers into any workflow. As I see it, the providers attributes are a default set of providers that we add to all/most workflows. But wavelength_range_from_chopper_frames only makes sense for 'analytical' unwrap workflows, not when we load an LUT from file. So you should add that provider in the specific workflow functions, not here. (And then we need a different provider for other workflows.)
There was a problem hiding this comment.
Yes that makes sense, I'll move it to the beer workflows. And maybe the wavelength range provider needs to change for it to work with the simulated lookup tables as well.
| """ | ||
| if nbins < 1: | ||
| raise ValueError(f'DspacingNBins must be positive, got {nbins}.') | ||
| dspacing_range = scn.conversion.tof.dspacing_from_wavelength( |
There was a problem hiding this comment.
Can you use the ElasticCoordTransformGraph instead? (Same as computation of DetectorTwoTheta) Currently, this conversion does not react to customisations in that graph.
There was a problem hiding this comment.
I am a little sceptical that this approach will work robustly in practice.
1. Compute range from chopper params
If you compute WavelengthRange from chopper parameters, you can end up with a range that is larger than what the source and guide can deliver. Then you end up with counts=0 bins even though you did not measure there. So you don't know if the d-spacing really should be zero.
2. Compute range from events
If you have an unwrap workflow other than the 'analytical' workflow, you need an alternative to computing WavelengthRange. If you compute it from the wavelength LUT, you get the same issue as above.
If you compute it from the measured events, you get an inconsistency between the sample run and auxiliary runs (vanadium, empty instrument, etc) or between different sample runs. Since this PR derives DspacingBins only from the sample run, you can get the same issue as in point 1 for the auxiliary runs.
If you would compute the bins for each run separately, you can get problems in the normalisation when the vanadium range is smaller than the sample range.
As far as I can tell, you can only mitigate point 1 via a simulation. And point 2 by including the measured ranges of all runs.
I think the best solution is to have the user enter a fixed wavelength range.
Where does this requirement come from? Is it really too much to ask to specify a range instead of a number of bins?
There was a problem hiding this comment.
This requirement comes from the Beer team and Celine.
- I don't see why having a range that is slightly larger than what the instrument can deliver would be a problem?
If we cannot measure there the counts will be zero, but the normalized counts will be nan.
As long as we cover the relevant region, having bins with intensity zero, or normalized intensity nan is not a significant issue (besides perhaps the inconvenience of exclude those when exporting).
- The way we compute the wavelength range should be agnostic of the approach we use for frame unwrapping, be that analytical or simulated. This is in principle not an issue, but the current implementation here need some adjustments.
In my opinion the simplest way to handle "mismatch" between sample and vanadium etc is either: a) making sure that the same instrument settings lead to the same wavelength and theta ranges, b) re-using the binning from the sample histogram for the normalization factor.
The user will always have the option to select the range themselves. This selection is "best effort" only, but I'd expect it to be sufficient for most cases.
There was a problem hiding this comment.
Can you check with the instrument team how they want to treat out or bounds bins? I don't want to guess and cause painful debugging sessions later.
If we cannot measure there the counts will be zero, but the normalized counts will be nan.
The vanadium normalisation will insert a mask for those bins and the XYE file writer will raise when there is a mask. So the data needs to be post-processed in that case.
a) making sure that the same instrument settings lead to the same wavelength and theta ranges
We get this when deducing the range from the choppers, so that is good because
b) re-using the binning from the sample histogram for the normalization factor.
If we pre-reduce the vanadium and save it to disk (a common case), we might get it as a histogram. In that case, we'd have to rebin and lose some precision. So while the vanadium norm function does this automatically, we should avoid this case if possible.
This branch has not been deployed
2 participants
Fixes #745