[ESSSPECTROSCOPY] Source the BIFROST Bragg peak monitor from the NXmonitor - #698
Draft
SimonHeybrock wants to merge 2 commits into
Draft
[ESSSPECTROSCOPY] Source the BIFROST Bragg peak monitor from the NXmonitor#698SimonHeybrock wants to merge 2 commits into
SimonHeybrock wants to merge 2 commits into
Conversation
The Bragg peak monitor is BIFROST's elastic monitor (cbm5), written as an
NXmonitor in CODA files, in the pinned geometry artifacts, and in the McStas
simulation file alike. The single-crystal workflow instead required it as
NeXusComponent[NXdetector], so loading raised
ValueError: The NeXus group 'elastic_monitor' was expected to be a
NXdetector but is a NXmonitor
against every real file. Only the user guide worked, by standing a detector
triplet in for the monitor.
Take the component, its transformation and its event data from ElasticMonitor,
which the workflow already declares in monitor_types, so all the loading nodes
exist. A monitor has no pixel offsets, so the position is the transformed
origin as in get_calibrated_monitor rather than compute_detector_position, and
no detector_number, so the single pixel is named explicitly for event assembly.
The Analyzer dependency is dropped: get_base_calibrated_detector_bifrost never
reads it, and a monitor in the direct beam has no analyzer.
Ltotal is a straight line for the same reason, so insert that provider rather
than leaving every caller to supply it.
Fix a latent defect this exposes: a time-dependent position makes ltotal depend
on 'time', but group_by_rotation has already renamed that same dimension to
'a4', so the broadcast in detector_wavelength_data rejected it. This bites
whenever the tank rotates; the user guide escaped it only because its stand-in
position is static.
The simulated data contains no Bragg peak monitor, so the detector stand-in
moves to simulation_providers and keeps the user guide working.
Streamed monitor events carry no event_id, so assemble_detector_data cannot group them. The monitor is a single pixel, so assign its geometry onto the events as assemble_monitor_data does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The BIFROST Bragg peak monitor workflow could not run against any real file.
get_calibrated_bragg_peak_detectorrequiredNeXusComponent[NXdetector], but the Bragg peak monitor is the elastic monitor (cbm5), written as anNXmonitorin CODA files, in the geometry artifacts and in the McStas simulation file alike, so loading raisedOnly the user guide worked, and only because it stands a detector triplet in for the monitor — it carries an explicit warning saying so.
The monitor's component, transformation and event data now come from
ElasticMonitor. The workflow already declaresmonitor_types=(ElasticMonitor, NormalizationMonitor), so every loading node exists and this is a re-pointing rather than new machinery. A monitor has no pixel offsets, so the position is the transformed origin as inget_calibrated_monitor, and nodetector_number, so the single pixel is named explicitly for event assembly. TheAnalyzerdependency is dropped:get_base_calibrated_detector_bifrostnever reads it, and a monitor in the direct beam has no analyzer.Event assembly no longer goes through
assemble_detector_data, which groups byevent_id. Streamed monitor events carry none — the file-based path hid this because CODA'scbm5_eventsdoes haveevent_id. It was only found by running the live path.Along the way this fixes a latent defect that is not specific to the monitor: a time-dependent position makes
ltotalcarrytime, butgroup_by_rotationhas already renamed that dimension toa4, so the broadcast indetector_wavelength_datarejected it. This bites whenever the tank rotates. It stayed hidden because the user guide's stand-in position is static, and because the inelastic/Q-cut path uses a differentdetector_wavelength_datathat takes noltotalat all.Testing
18/18 essspectroscopy tests pass and the user guide still computes (its
EmptyDetectorstand-in moved tosimulation_providers).There is no regression test for the monitor path here: the simulated data contains no Bragg peak monitor with event data — the simulation file's
elastic_monitoris histogram-mode. The path is instead exercised end to end downstream in scipp/esslivedata#1236, where streamed monitor events plus the live rotation readbacks produce a populated (Qpar, Qperp) map. That branch does not work against released essspectroscopy, so it is blocked on this PR.Open questions for reviewers
The scientists asked for the map in the laboratory frame, but
project_momentum_transferprojectssample_table_momentum_transfer— the sample-table frame, which is what you would want to compare against an expected reciprocal lattice. Worth settling before merge.single_crystal/detector.pyimports_assign_detector_positionfrom the sibling..detector; intra-package but private. Say the word if you want it promoted.