Fix false dependency of OutputCalibrationData on CorrectedDetector#250
Open
SimonHeybrock wants to merge 2 commits intomainfrom
Open
Fix false dependency of OutputCalibrationData on CorrectedDetector#250SimonHeybrock wants to merge 2 commits intomainfrom
SimonHeybrock wants to merge 2 commits intomainfrom
Conversation
…ctor assemble_output_calibration only uses Ltotal and two_theta, which are purely geometric quantities derived from detector pixel positions. By depending on CorrectedDetector[SampleRun], it unnecessarily pulled in the full event processing chain, blocking use in streaming contexts where event data is not available during finalization. Introduce DetectorTwoTheta sciline node type (mirroring the existing DetectorLtotal from ess.reduce) and a provider that computes it from EmptyDetector + beamline geometry. Change assemble_output_calibration to depend on DetectorLtotal[SampleRun] and DetectorTwoTheta[SampleRun] instead. Closes #249 Prompt: Please use a new branch and address #249. We concluded that using EmptyDetector should work, but hopefully tests will tell (check we have tests that are sensitive to this). Follow-up: Find out how the ess.reduce GenericTofWorkflow handles Ltotal. Is it already a node in the graph, or is it always embedded as a coord? If a node, we should consider doing the same for two_theta. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jl-wynen
reviewed
Feb 27, 2026
src/ess/powder/calibration.py
Outdated
| 'source_position': lambda: source_position, | ||
| 'sample_position': lambda: sample_position, | ||
| 'gravity': lambda: gravity, | ||
| } |
Member
There was a problem hiding this comment.
Please change this to use ElasticCoordTransformGraph instead of constructing a custom graph so that we have a single customisation point.
| two_theta: DetectorTwoTheta[SampleRun], | ||
| ) -> OutputCalibrationData: | ||
| """Construct output calibration data from average pixel positions.""" | ||
| # Use nanmean because pixels without events have position=NaN. |
- Use ElasticCoordTransformGraph in detector_two_theta instead of constructing a custom graph, providing a single customization point - Restore nanmean comment in assemble_output_calibration Prompt: We need to address review comments on PR #250.
jl-wynen
approved these changes
Feb 27, 2026
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
2 participants
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.
Summary
assemble_output_calibrationonly usesLtotalandtwo_theta— purely geometric quantities derived from detector pixel positions — yet it depended onCorrectedDetector[SampleRun], pulling in the full event processing chainOutputCalibrationDatain streaming contexts (esslivedata) where event data is unavailable during finalizationDetectorTwoThetasciline node type (mirroring the existingDetectorLtotalfromess.reduce) with a provider computing it fromEmptyDetector+ beamline geometryassemble_output_calibrationto depend onDetectorLtotal[SampleRun]andDetectorTwoTheta[SampleRun]insteadMotivation
See #249. In
ess.reduce.streaming.StreamProcessor,OutputCalibrationDataended up in the dynamic subgraph becauseCorrectedDetectordepends onNeXusData. This prevented computingIntensityTofduring finalization. The fix breaks this false dependency soOutputCalibrationDatacan be computed from static geometry alone.Whether
DetectorTwoThetashould be moved toess.reduce(next toDetectorLtotal) is tracked in scipp/ess#168.Closes #249
🤖 Generated with Claude Code