Transfer PV-logs during load of grouping and mask workspaces #539
Conversation
This file contains 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
This is a supporting PR for the reduction live-data work. The changes here allow all loaded workspaces, with the exception of the input data workspace itself, to be reused from reduction cycle to reduction cycle. During development work, it was found that although Mantid's `LoadDiffCal` allows an instrument to be specified, it does not additionally transfer the relevant instrument PV-logs from the donor workspace. The result of this is that any mask or grouping workspace loaded by SNAPRed would not have a defined stateID-SHA. During the reduction process, pixel-mask compatibility is checked using the combination of the stateID-SHA, and the lite-mode state -- ADS-resident pixel-mask workspaces with no stateID-SHA will be skipped as incompatible. Also found during development work, when the detector positions are compared between workspaces written using `SaveNexus` and read back using `LoadNexus`, they fail to match to the Mantid position and rotation target accuracy. For positions, this accuracy is `1.0e-9`m, and for rotations, it is `1.0e-9`m at `1000.0`m, which corresponds to `1.0e-12` radians. The reason for this readback mismatch has to do with the lack of a precision specification when writing the locations to the _legacy_ 'instrument_parameter_map' (in the NeXus file). Unfortunately, this same legacy system is used to initialize the instrument parameters during reload. This write-precision defect has been separately fixed in another Mantid PR. This current PR adds a temporary work around for this issue. This commit includes the following changes: * `LoadCalibrationWorkspaces` and `LoadGroupingDefinition` algorithms, both of which use Mantid's `LoadDiffCal` algorithm, now transfer the relevent instrument PV-logs to their output workspaces; * Any `FetchGroceriesAlgorithm` loader which goes through the `LoadNexus` pathway, now additionally calls `populateInstrumentParameters` for the output workspace. This bypasses the inaccuracy of the legacy 'instrument_parameter_map', and directly utilizes the PV-log values, which are stored in their binary representation as double-precision values; * General utilities `transferInstrumentPVLogs` and `populateInstrumentParameters` are provided in `snapred.backend.data.util.PV_logs_util`. The `populateInstrumentParameters` utility is a temporary work around, as `ExperimentInfo.populateInstrumentParameters` has now been exposed to the Python API in another Mantid PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #539 +/- ##
==========================================
- Coverage 95.98% 95.91% -0.07%
==========================================
Files 68 69 +1
Lines 5152 5163 +11
==========================================
+ Hits 4945 4952 +7
- Misses 207 211 +4 ☔ View full report in Codecov by Sentry. |
ekapadi
force-pushed
the
EWM9137_instrument_PV_logs
branch
from
0517849 to
4e92b71
Compare
for more information, see https://pre-commit.ci
rboston628
reviewed
Jan 31, 2025
| from mantid.simpleapi import AddSampleLog, mtd | ||
|
|
||
|
|
||
| def transferInstrumentPVLogs(dest: Run, src: Run, keys: Iterable): |
There was a problem hiding this comment.
Should this require the Run objects, or would it be more idiomatic to pass the workspace string names?
There was a problem hiding this comment.
If Run, could some kind of parameter validation be added?
Comment on lines
+23
to
+39
| CreateSampleWorkspace( | ||
| OutputWorkspace=wsName, | ||
| # WorkspaceType="Histogram", | ||
| Function="User Defined", | ||
| UserDefinedFunction="name=Gaussian,Height=10,PeakCentre=1.2,Sigma=0.2", | ||
| Xmin=0, | ||
| Xmax=5, | ||
| BinWidth=0.001, | ||
| XUnit="dSpacing", | ||
| NumBanks=4, # must produce same number of pixels as fake instrument | ||
| BankPixelWidth=2, # each bank has 4 pixels, 4 banks, 16 total | ||
| ) | ||
| LoadInstrument( | ||
| Workspace=wsName, | ||
| Filename=Resource.getPath("inputs/testInstrument/fakeSNAP_Definition.xml"), | ||
| RewriteSpectraMap=True, | ||
| ) |
There was a problem hiding this comment.
Isn't there a helper for this?
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.
Description of work
This is a supporting PR for the reduction live-data work. The changes here allow all loaded workspaces, with the exception of the input data workspace itself, to be reused from reduction cycle to reduction cycle.
During development work, it was found that although Mantid's
LoadDiffCalallows an instrument to be specified, it does not additionally transfer the relevant instrument PV-logs from the donor workspace. The result of this is that any mask or grouping workspace loaded by SNAPRed would not have a defined stateID-SHA. During the reduction process, pixel-mask compatibility is checked using the combination of the stateID-SHA, and the lite-mode state -- ADS-resident pixel-mask workspaces with no stateID-SHA will be skipped as incompatible.Also found during development work, when the detector positions are compared between workspaces written using
SaveNexusand read back usingLoadNexus, they fail to match to the Mantid position and rotation target accuracy. For positions, this accuracy is1.0e-9m, and for rotations, it is1.0e-9m at1000.0m, which corresponds to1.0e-12radians. The reason for this readback mismatch has to do with the lack of a precision specification when writing the locations to the legacy 'instrument_parameter_map' (in the NeXus file). Unfortunately, this same legacy system is used to initialize the instrument parameters during reload. This write-precision defect has been separately fixed in another Mantid PR. This current PR adds a temporary work around for this issue.Explanation of work
This commit includes the following changes:
LoadCalibrationWorkspacesandLoadGroupingDefinitionalgorithms, both of which use Mantid'sLoadDiffCalalgorithm, now transfer the relevent instrument PV-logs to their output workspaces;Any
FetchGroceriesAlgorithmloader which goes through theLoadNexuspathway, now additionally callspopulateInstrumentParametersfor the output workspace. This bypasses the inaccuracy of the legacy 'instrument_parameter_map', and directly utilizes the PV-log values, which are stored in their binary representation as double-precision values;General utilities
transferInstrumentPVLogsandpopulateInstrumentParametersare provided insnapred.backend.data.util.PV_logs_util. ThepopulateInstrumentParametersutility is a temporary work around, asExperimentInfo.populateInstrumentParametershas now been exposed to the Python API in another Mantid PR.To test
Additional unit tests have been added to test the new
PV_logs_util.Current unit tests have also been adjusted to cover this work. Mostly these adjustments involved adding a more-complete instrument to initialization to the workspaces used in the
GroceryServicetests.CIS testing
This is a SNAPRed internal item. It can be evaluated as part of the reduction live-data work.
Link to EWM item
Note that there is NO EWM item for this PR specifically. These defects were fixed in support of the reduction live-data work.
EWM#7437