[Essnmx] Use time bin width instead of number of bin edges. by YooSunYoung · Pull Request #264 · scipp/ess

YooSunYoung · 2026-03-24T16:22:35Z

Now you can set --time-bin-width instead of nbins.
I didn't remove the nbins option so you can still use it, but you need to set the --time-bin-width 0.

Command Example

essnmx-reduce --input-file nmx.hdf --time-bin-width 3 --time-bin-unit ms
# Or
essnmx-reduce --input-file nmx.hdf --time-bin-width 0 --nbins 50

In this case below, the nbins will be ignored and default time-bin-width of 3[ms] will be used.

essnmx-reduce --input-file nmx.hdf --nbins 50

Command Helper

essnmx-reduce -h

Workflow Configuration:
  --time-bin-coordinate {event_time_offset,time_of_flight}
                        Coordinate to bin the time data. Selecting `event_time_offset` means reduction steps are skipped, i.e. calculating `time of flight(tof)` and simply saves histograms of the raw data. (default: time_of_flight)
  --time-bin-width TIME_BIN_WIDTH
                        Width(Length) of each Time Bin in [time_bin_unit]. If `time_bin_width` and `nbins` are both given, `time_bin_width` will be preferred. Set it to `0` if you want to use `nbins` instead. (default: 3)
  --nbins NBINS         Number of Time bins. If `bin_width` is given, `nbins` will be ignored. (default: 50)

aaronfinke · 2026-03-24T16:41:26Z

--time-bin-width and --nbins should be mutually exclusive options (e.g. use ArgumentParser.add_mutually_exclusive_group() )

The essnmx-reduce output should include the number of generated bins and also (this would be nice) number of events per bin.

YooSunYoung · 2026-03-24T16:41:32Z

The failing tests seem to be related to the resource issue....?? It doesn't fail on my workstation... I'll have a look at it tmr...

YooSunYoung · 2026-03-24T16:44:15Z

@aaronfinke

--time-bin-width and --nbins should be mutually exclusive options (e.g. use ArgumentParser.add_mutually_exclusive_group() )

Okay...!

The essnmx-reduce output should include the number of generated bins and also (this would be nice) number of events per bin.

You mean the logging/printing them out?
Or you want an additional 1-D histogram in the output file?

aaronfinke · 2026-03-24T16:53:17Z

@YooSunYoung

You mean the logging/printing them out?
Or you want an additional 1-D histogram in the output file?

Certainly as logger output, yes, but also a cute 1D histogram would also be useful. dials.find_spots does something similar in its output:

Histogram of per-image spot count for imageset 0:
1879 spots found on 49 images (max 78 / bin)
                     *                           
                     *                           
                    **                          *
               **   ***              *          *
      *   ** * *** ******   * * **   *          *
      ****** ************* ** * **   ***        *
     ******* ****************** **   ***   **** *
******************************* **** *** ********
*************************************************
*************************************************
1                    image                     49

aaronfinke · 2026-03-24T16:58:55Z

DIALS code for making that histogram:

https://github.com/dials/dials/blob/main/src/dials/util/ascii_art.py

YooSunYoung · 2026-03-25T14:46:28Z

@aaronfinke
Now it's not allowed to give both nbins and time-bin-width. I'll add the 1D histogram too (#267).

pixi run essnmx-reduce --nbins 3 --time-bin-width 3
usage: essnmx-reduce [-h] --input-file INPUT_FILE [INPUT_FILE ...] [--swmr] [--detector-ids DETECTOR_IDS [DETECTOR_IDS ...]] [--iter-chunk] [--chunk-size-pulse CHUNK_SIZE_PULSE] [--chunk-size-events CHUNK_SIZE_EVENTS]
                     [--time-bin-coordinate {event_time_offset,time_of_flight}] [--min-time-bin MIN_TIME_BIN] [--max-time-bin MAX_TIME_BIN] [--time-bin-unit {ms,us,ns}] [--lookup-table-file-path LOOKUP_TABLE_FILE_PATH]
                     [--tof-simulation-num-neutrons TOF_SIMULATION_NUM_NEUTRONS] [--tof-simulation-min-wavelength TOF_SIMULATION_MIN_WAVELENGTH] [--tof-simulation-max-wavelength TOF_SIMULATION_MAX_WAVELENGTH]
                     [--tof-simulation-min-ltotal TOF_SIMULATION_MIN_LTOTAL] [--tof-simulation-max-ltotal TOF_SIMULATION_MAX_LTOTAL] [--tof-simulation-seed TOF_SIMULATION_SEED] [--time-bin-width TIME_BIN_WIDTH | --nbins NBINS] [--verbose] [--skip-file-output]
                     [--output-file OUTPUT_FILE] [--overwrite] [--compression {NONE,GZIP,BITSHUFFLE_LZ4}]
essnmx-reduce: error: argument --time-bin-width: not allowed with argument --nbins

YooSunYoung · 2026-03-27T07:14:34Z

I just realized... we probably want to set the bid width in angstrom

aaronfinke · 2026-03-27T08:40:42Z

I just realized... we probably want to set the bid width in angstrom

Not necessarily. Setting by TOF is more intuitive. I would keep it as seconds or milliseconds.

packages/essnmx/src/ess/nmx/executables.py

nvaytet · 2026-04-08T12:24:20Z

packages/essnmx/src/ess/nmx/executables.py

+            dim=t_coord_name,
+            start=min_t.to(unit=wf_config.time_bin_unit),
+            stop=max_t.to(unit=wf_config.time_bin_unit),
+            step=time_bin_width,


I think we need to be careful with using arange here. Depending on the step size, you can be dropping the max edge.
For example, np.arange(1, 17.5, 3) yields array([ 1., 4., 7., 10., 13., 16.]). So the edges would stop at 16, missing everything after that.
Similarly, it could be surprising that np.arange(2, 18., 2) stops at 16, while np.arange(2, 18.01, 2) stops at 18...

Hmm yeah... we might not want to lose the last bin... should I just calculate the number of bins and correct last edge, and use linspace instead?

Well we / @aaronfinke need to decide which is the quantity that should be preserved.
If we specify a range with min and max, and a bin width, but the range does not span an integer number of bin widths, should we:

preserve the bin width and adjust the start or end of the range with some padding to allow an integer number of bin widths

preserve the range and compute a number of bins based on the requested width, use linspace, and end up with a bin width which will not exactly match the requested width

Something else?

My immediate reaction is that setting the bin length to the user defined value is more important than ensuring all bin sizes are equal, so I'd go for option 1. So setting the nbins as $floor(\Delta_{TOF}/t_{bin})$ and setting bin[0] and bin[-1] size as $t_{bin}$ + $(\Delta_{TOF}$ $modulo$ $t_{bin})/2$ makes the most sense.

We just talked offline and the conclusion was to keep the bin-width as the user-input and add an extra padding bin if needed.

I'll fix this in this way and ask for a review again.

packages/essnmx/tests/executable_test.py

nvaytet · 2026-04-08T12:38:59Z

packages/essnmx/tests/executable_test.py

+    # i.e. if the look up table wavelength rage changes,
+    # the number of bins changes and the histogram data sizes changes.
+    # This test is only for checking if the look up table is used as expected or not
+    # therefore using number of bins should be fine.


Not sure I understood why changes in the resolution/range in the lookup table would cause changes in number of bins in the reduced result? What did I miss?

Because the wavelength range of reduced data changes if the lookup table wavelength range changes.
If the wavelength range shrinks, the number of bins should be smaller.

nvaytet · 2026-04-08T12:40:42Z

packages/essnmx/tests/executable_test.py

        default_model = default_child.model_dump(mode='python')
        for key, testing_value in testing_model.items():
-            if key == 'lookup_table_file_path':
+            if key in ['lookup_table_file_path', 'nbins']:


Shouldn't this also include time_bin_width? It can also be None, right?

nvaytet · 2026-04-08T12:44:54Z

packages/essnmx/src/ess/nmx/executables.py

+        histogram = tof_da.hist({t_coord_name: t_bin_edges.to(unit=t_coord_unit)})
+        tof_histograms[detector_name] = histogram
+
+    _tof_histogram = next(iter(tof_histograms.values()))


A bit annoying that we have to do this, instead of just using the t_bin_edges below.
I understand it's because of

histogram = tof_da.hist({t_coord_name: t_bin_edges.to(unit=t_coord_unit)})

above.

Does that mean that the t_coord_unit can be different for different detector panels? How likely is that?

Is the unit of the tof coordinate in the reduced data something the user can control via one of the args?
If so, maybe the provider that computes the tof should do the conversion to that unit, and then we would know the unit from the argument and could avoid some gymnastics here?

Being able to say whether you want the output in microseconds or milliseconds sounds like something the users might want?

Does that mean that the t_coord_unit can be different for different detector panels? How likely is that?

No... it was just me being lazy to do it only once...

But in theory they could have different units I guess. Although we probably want same units in the output.

Being able to say whether you want the output in microseconds or milliseconds sounds like something the users might want?

Probably...? Then it's easy for us to decide the unit here.

packages/essnmx/tests/executable_test.py

Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com> Co-authored-by: Sunyoung Yoo <17974113+YooSunYoung@users.noreply.github.com>

YooSunYoung · 2026-04-09T14:15:14Z

packages/essnmx/src/ess/nmx/executables.py

+        if bin_edges[t_coord_name, -1] < max_t:
+            # Need to append one more edge to cover the whole range.
+            true_last_bin_edge = bin_edges[t_coord_name, -1] + time_bin_width
+            bin_edges = sc.concat([bin_edges, true_last_bin_edge], dim=t_coord_name)


This was easier to read/write than calculating linspace nbins start stop...

YooSunYoung mentioned this pull request Mar 25, 2026

[essnmx] Print/log/dump 1-D histogram along the time dimension. #267

Open

nvaytet added essnmx Issues for essnmx. labels Mar 25, 2026

nvaytet changed the title ~~Use time bin width instead of number of bin edges.~~ [Essnmx] Use time bin width instead of number of bin edges. Mar 25, 2026

YooSunYoung force-pushed the essnmx-bin-width branch from a74a17f to c4a32dd Compare March 27, 2026 08:30

YooSunYoung marked this pull request as draft March 27, 2026 08:31

YooSunYoung marked this pull request as ready for review March 27, 2026 10:15

YooSunYoung added this to Development Board Mar 27, 2026

YooSunYoung moved this to In progress in Development Board Mar 27, 2026

YooSunYoung added 3 commits April 8, 2026 09:09

essnmx-bin-width

6a62562

Use time bin width instead of number of bin edges.

a9de5ec

Make timebinwidth and nbins mutually exclusive option.

dbff8c3

YooSunYoung force-pushed the essnmx-bin-width branch from c4a32dd to dbff8c3 Compare April 8, 2026 07:09

YooSunYoung requested a review from nvaytet April 8, 2026 07:15

nvaytet reviewed Apr 8, 2026

View reviewed changes

YooSunYoung commented Apr 9, 2026

View reviewed changes

packages/essnmx/tests/executable_test.py Outdated Show resolved Hide resolved

YooSunYoung and others added 5 commits April 9, 2026 09:51

Apply suggestions from code review

8008cfd

Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com> Co-authored-by: Sunyoung Yoo <17974113+YooSunYoung@users.noreply.github.com>

Apply suggestions from code review

0037d02

Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com> Co-authored-by: Sunyoung Yoo <17974113+YooSunYoung@users.noreply.github.com>

Check if the bin edges cover the whole range of the time coordinate.

c5bc6d6

Fix bin edge building logic [skip ci]

538b90d

Fix bin edge building logic [skip ci]

f50bd6b

YooSunYoung commented Apr 9, 2026

View reviewed changes

Conversation

YooSunYoung commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Command Example

Command Helper

Uh oh!

aaronfinke commented Mar 24, 2026

Uh oh!

YooSunYoung commented Mar 24, 2026

Uh oh!

YooSunYoung commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronfinke commented Mar 24, 2026

Uh oh!

aaronfinke commented Mar 24, 2026

Uh oh!

YooSunYoung commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YooSunYoung commented Mar 27, 2026

Uh oh!

aaronfinke commented Mar 27, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YooSunYoung commented Mar 24, 2026 •

edited

Loading

YooSunYoung commented Mar 24, 2026 •

edited

Loading

YooSunYoung commented Mar 25, 2026 •

edited

Loading