fix: convert unlimited TOA to expected range

[jokasimr]

Fixes one aspect of #161, specifically it converts the monitor "tof" values to the event_time_offset range that we expect them to have.

[jl-wynen]

What is the range in the file?

[jl-wynen]

This seems very complicated for what is effectively a modulus operation. How about something like

tof, counts, err = np.loadtxt(file_path, usecols=(0, 1, 2), unpack=True)
toa = tof % sc.reciprocal(sc.scalar(14.0, unit='Hz'))
toa = _to_edges(sc.array(dims=["toa"], values=toa, unit="us"))
data = sc.DataArray(
    sc.array(dims=["toa"], values=counts, variances=err**2, unit="counts"),
    coords={
        "toa": toa,
    },
)
data = data.hist(toa=some_range_based_on(tof))

[jokasimr]

That doesn't work. You'll get multiple values added to some bins, and no values added to others. You can try it out and look at the difference.

[nvaytet]

I also didn't really follow. From the Strategy outlined, it sounded like a modulo operation?

[jokasimr]

You're not wrong, it's similar. But if you use the suggested approach of applying mod and then re-histogramming you get aliasing effects.

If your final grid is sufficiently coarse then this is not very visible, but we don't want to restrict the monitor histogram to a very coarse grid directly after loading the data.

[nvaytet]

Yeah I think you're right; the important point here is that the input data is histogrammed, right?
Then it's not as simple as just a modulo...

Are we sure that it will always be histogrammed or do we need to check for da.bins is None?

[jokasimr]

the important point here is that the input data is histogrammed, right?

Yes the aliasing problem only affects histogrammed data.

Are we sure that it will always be histogrammed or do we need to check for da.bins is None?

In this case we're loading data from a simulation. The current version of the code assumes the monitor is a histogram, I think it's fine to keep that assumption for now. If they change the simulation in the to have an event mode monitor we can deal with that later.

[nvaytet]

Can you just add a safety net with

if da.bins is not None:
    raise NotImplementedError

?

[jokasimr]

I think there migh be a misunderstanding here. What's your concern?

The monitordata is loaded from a CSV file, it can never be binned the way the program works now.

[nvaytet]

I'm concerned about someone in the future trying to use this to load a different file where the monitor data would be events?
Cf your comment:

If they change the simulation in the to have an event mode monitor we can deal with that later.

[jokasimr]

I really don't think we need to worry about that.

A non public aux function that is used in a single place should not validate it's arguments. It's only extracted from the place where it is called to improve readability.
We should assume that it will not be re-used blindly for some purpose that it was not meant for in some future context that we cannot predict.
Adding validation to it seems to me the same as adding unnecessary validation to a piece of code for the reason that it might be copied and used erroneously.

I don't want this to be a long discussion. If you think this is important, please come talk to me in person instead to avoid misunderstandings.

[jokasimr]

What is the range in the file?

The range in the file is 0 to 260 ms, so ~4 pulse periods, but in most of that range the monitor intensity is zero.

[nvaytet]

I'm getting confused here between TOA=time-of-arrival and ETO=event_time_offset.
ETO is the time which will always be between 0 and 1/14s.
TOA is the time the neutron arrived at the detector, so TOF = TOA - birth time of the neutron. TOA can have any value.

Which one are we refering to here?

[jokasimr]

In the CSV files we get TOA. But the time-of-flight lookup table expects ETO. So to be able to compute TOF using the time-of-flight lookup table we need to convert TOA to ETO.

For the detector data this is easy, it's just a mod operator on the TOA values in the file.

For (histogramed) monitor data, it's a bit more involved, but essentially the same in principle.

[jl-wynen]

I ran the user guide notebooks and most of it looks basically unchanged. But the I(d) plot in the basic user guide is bad now:

So I think something is wrong with this approach. Can you investigate?

[jokasimr]

I ran the user guide notebooks and most of it looks basically unchanged. But the I(d) plot in the basic user guide is bad now:

The cause is that normalize_by_monitor_histogram does not take into account the case when the monitor histogram is 0. This causes division by zero for some wavelength values, and that removes the intensity from all dspacing bins that has contributions from that wavelength value.

The exact reason why there exist some zero intensity bins is the MonitorTofData after this change, but not before, is unclear to me. But I think it has to do with the details of how the TofLookupTable is applied to histogrammed data. See https://github.com/scipp/essreduce/blob/5391cd2c264ffec7cbb0990cdef526266e762c04/src/ess/reduce/time_of_flight/eto_to_tof.py#L94.

[jokasimr]

When masking zero monitor regions in the monitor normalization function, the I(d) plot in the basic user guide looks like this:

[jl-wynen]

Can you make a plot with both the new and old I(d)?

[jokasimr]

Solid blue is old, and transparent blue is new.

There are some differences in the high-dspacing region, but that is expected since before we were dropping monitor intensity for the longest wavelengths.