Expose NeXus transformation chains in workflow steps #114
Conversation
unused arg Compute component pos later Fix for non-nexus component "groups"
| return compute_component_position(loaded) | ||
| return loaded |
There was a problem hiding this comment.
This is the heart of the change in this PR: We are no longer computing the position directly here. This is moved into an extra step.
Previously we introduced false dependencies of detector ids on these.
| @@ -95,9 +91,9 @@ class TransmissionRun(Generic[ScatteringRunType]): | |||
| """Identifier for an arbitrary monitor""" | |||
| Monitor5 = NewType('Monitor5', int) | |||
| """Identifier for an arbitrary monitor""" | |||
| Incident = NewType('Incident', int) | |||
| IncidentMonitor = NewType('IncidentMonitor', int) | |||
There was a problem hiding this comment.
Added the monitor suffix here since these now show up as MyComponent[IndicidentMonitor] and it may be confusing without.
| ) -> NeXusSource[RunType]: | ||
| """ | ||
| Load a NeXus source group from a file. | ||
| def nx_class_for_monitor() -> NeXusClass[MonitorType]: |
There was a problem hiding this comment.
Slightly annoying that we need to explicitly have nx_class_for_monitor, nx_class_for_detector, nx_class_for_source, nx_class_for_sample... but not sure what can be done?
There was a problem hiding this comment.
One could set it as workflow parameters. Alternative would be to duplicate downstream providers, but as those have functionality it would mean code duplication, i.e., break more easily by running out of sync.
There was a problem hiding this comment.
I think it's fine as is for now.
| ) -> NeXusMonitor[RunType, MonitorType]: | ||
| """ | ||
| Load monitor from NeXus, but with event data replaced by placeholders. | ||
| When loading a detector or monitor, event data is replaced by placeholders. |
There was a problem hiding this comment.
Maybe I missed something, but is this for loading only detectors and monitors? I thought Component was also for sample and source?
Edit: I now see that the case for NXsample is overloaded by the load_nexus_sample function above. Are we missing a load_nexus_source function?
There was a problem hiding this comment.
It is also used for the source. But this paragraph basically just applies to the groups that have event data.
| file_path_to_file_spec, | ||
| all_pulses, | ||
| component_spec_by_name, | ||
| unique_component_spec, # after component_spec_by_name, partially overrides |
There was a problem hiding this comment.
Does the comment mean that we are relying on the behaviour of the pipeline; that it applies the providers in the order that they are given. Can we assume that the behaviour will never change?
There was a problem hiding this comment.
We are doing that in many places now, not just here: Insertion order does matter.
There was a problem hiding this comment.
There are explicit tests for this, e.g., here: https://github.com/scipp/sciline/blob/a90b73c50d4cef1fc8adb231dc2208cf99ef2f0a/tests/pipeline_test.py#L916. That is, this is intentional behavior and not likely to change. If it would, quite a few other workflows would also break, I think.
| nexus_detector, | ||
| offset=workflow.no_offset, | ||
| source_position=source_position, | ||
| sample_position=workflow.origin, | ||
| gravity=workflow.gravity_vector_neg_y(), | ||
| bank_sizes={}, | ||
| ) | ||
| assert_identical( | ||
| detector.drop_coords(('sample_position', 'source_position', 'gravity')), | ||
| nexus_detector['data'], |
Part of #96.
This is not completing this task, but does most of the prep work. The idea is that the
to_transformationcould accept additional arguments (or be replaced by one that does), which could:Neither 1.) nor 2.) is implemented right now. The initial focus will be on 1.). I would mainly like input from the reviewer on whether this new structure is adequate for this. The actual implementation of these features could be performed in follow-up work.
Depends on a release of scipp/scippnexus#243 (that is why CI is failing for now, otherwise this PR is ready for review).
Note that I ran into major (code) overhead/duplication from the current duplication (or near-duplication) of different code for different component types. Previously we had failed to find a way to generalize this. The reason was that monitors required one extra type-parameter (
MyMonitor[RunType, MonitorType]vsMyDetector[RunType],MySample[RunType], ...). I believe I have now solved this via the newComponentType. We can thus useMyComponent[Component, RunType]almost throughout, resulting in a significant deduplication. The "trick" is thatComponenttakes general component names except for the monitors, where there are multiple named ones.