Use ess.reduce.nexus NeXus workflows as basis for all workflows
#160
Conversation
| def to_detector_position_offset( | ||
| global_offset: DetectorBankOffset, beam_center: BeamCenter | ||
| ) -> DetectorPositionOffset[RunType]: | ||
| return DetectorPositionOffset[RunType](global_offset - beam_center) | ||
|
|
||
|
|
||
| def to_monitor_position_offset( | ||
| global_offset: MonitorOffset[MonitorType], | ||
| ) -> MonitorPositionOffset[RunType, MonitorType]: | ||
| return MonitorPositionOffset[RunType, MonitorType](global_offset) |
There was a problem hiding this comment.
These two helpers replace logic in the removed components.py, which was mostly duplicating more generic code.
| @@ -23,46 +21,6 @@ | |||
| CalibrationFilename = NewType('CalibrationFilename', str) | |||
|
|
|||
|
|
|||
| def read_xml_detector_masking(filename: PixelMaskFilename) -> MaskedDetectorIDs: | |||
There was a problem hiding this comment.
Moved to ess/sans: While strictly speaking ISIS-specific, our ess.loki unit tests were using this, introducing an annoying dependency of the ess.loki unit tests on ess.isissans.
| SourcePosition = reduce_gt.SourcePosition | ||
|
|
||
| DetectorBankSizes = reduce_t.DetectorBankSizes | ||
| NeXusDetectorName = reduce_t.NeXusDetectorName |
There was a problem hiding this comment.
I don't like this, but I think otherwise we would need to rewrite a lot more imports and type hints? This way users can still import from ess.sans.types and don't need to know which ones are defined in ess.reduce.nexus.... even though that is how the graph viz now displays them :(
There was a problem hiding this comment.
I know I am probably alone on this, but see this as another reason for considering #95 again.
There was a problem hiding this comment.
Has the situation changed now that we have workflow objects with default parameters set?
I guess the arguments about inspecting intermediate results have not really changed...
There was a problem hiding this comment.
You mean the default params we introduced for the widgets? That is not merged and probably still in flux, so I cannot tell yet.
SimonHeybrock
force-pushed
the
ess-reduce-nexus-workflow
branch
from
d0ea185
to
1195334
Compare
SimonHeybrock
force-pushed
the
ess-reduce-nexus-workflow
branch
from
1195334
to
4a497e9
Compare
| dg: LoadedFileContents[RunType], nexus_name: NeXusMonitorName[MonitorType] | ||
| ) -> RawMonitor[RunType, MonitorType]: | ||
| # See https://github.com/scipp/sciline/issues/52 why copy needed | ||
| mon = dg['monitors'][nexus_name]['data'].copy() |
There was a problem hiding this comment.
Is the need for this copy now gone? The issue is still open...
There was a problem hiding this comment.
I think we are not actually accessing a value property here?
| data_to_tof, | ||
| monitor_to_tof, | ||
| ) | ||
|
|
||
|
|
||
| def LokiAtLarmorWorkflow() -> sciline.Pipeline: |
There was a problem hiding this comment.
Does it make more sense to move this to the workflow.py file?
If so, this file would now be pretty empty. Can we remove it altogether? (move stuff to workflow.py or init.py?)
There was a problem hiding this comment.
It does, but there are also in-flight modifications in the widgets branch, and workflow.py actually contains beamlime-specifics which should be renamed. In other words, let us do this later.
| workflow = loki.LokiAtLarmorWorkflow() | ||
| workflow.insert(_hist_monitor_wavelength) | ||
| workflow[NeXusMonitorName[Incident]] = "monitor_1" | ||
| workflow[NeXusMonitorName[Transmission]] = "monitor_2" | ||
| workflow[WavelengthBins] = sc.linspace("wavelength", 1.0, 13.0, 50 + 1) |
| MaskedData[SampleRun], | ||
| NormWavelengthTerm[SampleRun], | ||
| ElasticCoordTransformGraph, | ||
| ) | ||
| workflow = workflow.copy() | ||
| # Avoid reshape of detector, which would break boolean-indexing by cost function | ||
| workflow[DetectorBankSizes] = {} |
| @@ -296,8 +277,7 @@ def test_beam_center_from_center_of_mass_is_close_to_verified_result(): | |||
|
|
|||
|
|
|||
| def test_phi_with_gravity(): | |||
| params = make_params() | |||
| pipeline = sciline.Pipeline(loki_providers(), params=params) | |||
| pipeline = make_workflow() | |||
| pipeline[BeamCenter] = _compute_beam_center() | |||
| pipeline[CorrectForGravity] = False | |||
| data_no_grav = pipeline.compute( | |||
There was a problem hiding this comment.
I skimmed through the tests, it seemed it was mostly about refactoring by making a fixture that would create the workflows. I don't think there were fundamental changes to any of the tests.
Is that correct?
I left a couple comments below to explain some of the refactoring.
CI will fail until ESSreduce is fixed and released.(done)