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

Ticket42 sum detect monitor spectra #66

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

esmith1729
Copy link

@esmith1729 esmith1729 linked an issue Nov 19, 2024 that may be closed by this pull request
5 tasks
Copy link
Contributor

@Tom-Willemsen Tom-Willemsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation & tests look good, mostly just some docs/clarity nitpicking.

Apologies for the number of comments - most of them are extremely minor - just worth trying to be precise I think.

Can you also merge in main as there's a merge conflict

@@ -205,6 +205,54 @@ Published signals:
- `reducer.mon_counts_stddev` - uncertainty (standard deviation) of the summed monitor counts
- `reducer.intensity_stddev` - uncertainty (standard deviation) of the normalised intensity

### Time of Flight and Wavelength Bounding Spectra

Scalar Normalizers (such as PeriodGoodFramesNormalizer, GoodFramesNormalizer) can be passed a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe you could link these to api docs? something like:

{py:obj}`ibex_bluesky_core.some.path.PeriodGoodFramesNormalizer`

should make a "nice" link to the API docs out of this.

- In either case, the bounds are passed as a scipp array, which needs a `dims` attribute, `values` passed
as a list, and `units` (μs/microseconds for time of flight bounding, and angstrom for wavelength bounding)

- If you don't specify either of these options, they will default to an unbounded spectra
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarity nit:

Suggested change
- If you don't specify either of these options, they will default to an unbounded spectra
- If you don't specify either of these options, they will default to summing over the entire spectrum.

import scipp

wavelength_bounds = scipp.array(dims=["tof"], values=[0.0, 5.1], unit=scipp.units.angstrom, dtype="float64")
beam_total = scipp.scalar(value=85.0, unit=sc.units.m),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarity nit:

beam_total is a bit ambiguous - maybe total_flight_path_length is clearer?

{py:obj}`ibex_bluesky_core.devices.simpledae.reducers.tof_bounded_spectra`


Monitor Normalizers, which have both a monitor as well as detector, can be passed a summing function for each of these components independently, e.g. the detector can use time of flight while the monitor uses wavelength. Here is an example with wavelength bounding used to sum the monitor component, and time of flight bounding for the detector summing spectra:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should: somewhere we need to explain that while you can use different flight path lengths for detectors & monitors, there is an assumption that the flight path length is the same for all detector pixels. This will usually be true for the kinds of detector pixels that are regularly scanned over.

(We could in future allow a length-per-pixel, but not as part of this ticket)

and monitor normalizers.

Both Scalar Normalizers (PeriodGoodFramesNormalizer, GoodFramesNormalizer) and MonitorNormalizers
provide the following:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
provide the following:
accept the following arguments:



def wavelength_bounded_spectra(
bounds: sc.Variable, beam_total: sc.Variable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

As per comment in docs earlier, I think beam_total is a little ambiguous, can we think of a clearer name? total_flight_path_length?


assert det_counts == 500 # 500 from first detector
assert mon_counts == 2000 # 2000 half of monitor first bin
assert intensity == pytest.approx(0.25) # 500 / 2500
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless I'm really bad at maths...

Suggested change
assert intensity == pytest.approx(0.25) # 500 / 2500
assert intensity == pytest.approx(0.25) # 500 / 2000

intensity = await monitor_normalizer_zero_to_one_half_det_norm_mon_tof.intensity.get_value()

assert det_counts == 9000.0 # 1 + 2 + 3 + 2 + 1 from detector = 9
assert mon_counts == 500.0 # 1k / 2k = 500 from monitor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert mon_counts == 500.0 # 1k / 2k = 500 from monitor
assert mon_counts == 500.0 # 1k / 2 = 500 from monitor

?

Comment on lines +97 to +98
if "tof" not in bounds.dims:
raise ValueError("Should contain tof dims")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could: In the other one we also check bounds have length 2, should that be done here too?

async def sum_spectra_with_wavelength(
spectra: Collection[DaeSpectra],
) -> sc.Variable | sc.DataArray:
"""Sum spectra and convert time of flight to wavelength."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we make this docstring a little more precise?

e.g.

Suggested change
"""Sum spectra and convert time of flight to wavelength."""
"""Sum a set of spectra between the specified wavelength bounds."""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sum detector/monitor spectra with tof/wavelength bounds
2 participants