User can manually configure tof or event_time_offset bin edges.#185
User can manually configure tof or event_time_offset bin edges.#185YooSunYoung merged 5 commits intomainfrom
Conversation
| f"{wf_config.time_bin_coordinate} values.\n" | ||
| "The histogram will all have zero values.", | ||
| category=UserWarning, | ||
| stacklevel=3, |
There was a problem hiding this comment.
Different stacklevel compared to above; was that intentional?
There was a problem hiding this comment.
no...! : D.... I'll fix them. Thanks.
| stacklevel=4, | ||
| ) | ||
| else: | ||
| min_t = da_min_t |
There was a problem hiding this comment.
I think you want to be careful with the max, because if you set it equal, then the last event will be dropped. See scipp/scipp#3566 for example.
You probably just want to add a nextafter to be safe.
|
|
||
| # Build the bin-edges to histogram the results. | ||
| n_edges = wf_config.nbins + 1 | ||
| return sc.linspace(dim=t_coord_name, start=min_t, stop=max_tof, num=n_edges) |
There was a problem hiding this comment.
Will users ever want to make log-spaced bins?
| ) | ||
|
|
||
|
|
||
| def test_histogram_event_time_offset(reduction_config: ReductionConfig) -> None: |
There was a problem hiding this comment.
You could consider parametrizing the tests to use both tof and eto coordinates to improve coverage?
There was a problem hiding this comment.
test_reduction_only_number_of_time_bins already uses the tof coordinate since it's the default setting.
There was a problem hiding this comment.
I guess I meant parametrize also the other tests:
test_histogram_invalid_min_max_raises, test_histogram_out_of_range_min_warns, test_histogram_out_of_range_max_warns etc.
They also call the top-level reduction and not the lower level function. This would just be to ensure that nothing specific to the name tof has crept into the _build_time_bin_edges function?
There was a problem hiding this comment.
I see...! I parametrized some of them. One of them, there's no overlapping range for eto/tof so I just added one more test using event_time_offset.
They also call the top-level reduction and not the lower level function. This would just be to ensure that nothing specific to the name tof has crept into the _build_time_bin_edges function?
Yeah and the _build_time_bin_edges is just an implementation detail that shouldn't be tested separately I thought.
Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>
|
@nvaytet Thank you for reviewing! I fixed the parts you pointed out. |
2 participants
Fixes scipp/ess#194
Before, we just had the names in the configuration that does nothing.
Now the time-bin-edges are decided based on the user-configuration.
Selection of time bin edges.
User can decide to use
event_time_offsetortime-of-flight(tof)to histogram the data.The former option means the output has histograms of all detectors without calculating time of flight or wavelength.
Setting time-bin-edges.
User can decide the bin edge - min/max and number of bins.
It both works for
event_time_offsetandtime-of-flight.reductionmethods checks if the settings are reasonable based on the data.It is because not like the
event_time_offset,the range of
time-of-flightcan change depends on the experiment settings.Therefore it is good to have a safety check.