feat: more flexible helper for batch reduction #155
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a more flexible helper for batch reduction by refactoring the dataset computation workflow and its associated tests. Key changes include:
- Replacing the function orso_datasets_from_measurements with a more versatile from_measurements that supports both list and mapping inputs.
- Adjusting the test cases to validate the new helper behavior and result types.
- Moving the RawChopper class from the amor module to the reflectometry types module and updating corresponding imports.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/tools_test.py | Updated tests to use from_measurements and verify expected parameters. |
| src/ess/reflectometry/workflow.py | Adjusted imports to use the new location of RawChopper and streamline workflow. |
| src/ess/reflectometry/types.py | Added RawChopper class to reflectometry/types to replace its previous location. |
| src/ess/reflectometry/tools.py | Renamed the dataset helper and refactored scaling logic with additional helper functions. |
| src/ess/amor/types.py | Removed duplicate RawChopper class. |
| src/ess/amor/load.py | Removed the import and reference to RawChopper. |
src/ess/reflectometry/tools.py
Outdated
| return datasets if names is None else dict(zip(names, datasets, strict=True)) | ||
|
|
||
|
|
||
| def _workflow_needs_quantity_A_even_if_quantitiy_B_is_set(workflow, A, B): |
There was a problem hiding this comment.
The function name '_workflow_needs_quantity_A_even_if_quantitiy_B_is_set' has a typo in 'quantitiy'. Consider correcting it to '_workflow_needs_quantity_A_even_if_quantity_B_is_set' for clarity.
| def _workflow_needs_quantity_A_even_if_quantitiy_B_is_set(workflow, A, B): | |
| def _workflow_needs_quantity_A_even_if_quantity_B_is_set(workflow, A, B): |
Copilot uses AI. Check for mistakes.
| wf[Filename[SampleRun]] = parameters[Filename[SampleRun]] | ||
| return wf | ||
|
|
||
| if scale_to_overlap: |
There was a problem hiding this comment.
As discussed during the standup, I don't really get why we need the helper function, as opposed to just having different workflows.
- You would have a workflow that computes a reflectivity curve.
- If you want to compute this for multiple runs, you just map over the run numbers or filenames, and then use
compute_mapped - If you want to
scale_to_overlap, you have a workflow that basically does the mapping and adds a provider that either just scales the results or scales and merges. You then compute a different result (e.g.CombinedScaledReflectivityOverQ)
You can then view the graph of what is being done for everything (because with the helper function you can't see it all, only pieces, and they are not easy to get to either if you just installed the package?)
In any case, we should avoid having a long discussion here on github, this was just to have a starting point for an in-person discussion.
There was a problem hiding this comment.
It's possible we can implement the same thing using sciline map reduce. Let us see the helper in this PR as a reference implementation and try to re-implement as one big workflow.
There was a problem hiding this comment.
After in person discussion, it seems difficult to avoid using a helper function to loop over files, and use a single large pipeline for all cases.
We are already using mapping to combine events from multiple files, and having a double mapping to map over different angles, but using the same mapping parameter Filename[SampleRun] would not work.
Having Filename as a scope with 2 params could maybe work, Filename[SampleRun, Angle], but that would require changing it in essreduce in the types common to all other workflows.
In addition, the scale_to_overlap sometimes wants to scale the event weights before the final 1d R(Q) result, and it is not as simple as just inserting a provider at the end of the workflow to apply the scaling.
With all this taken into account, the helper function seems like a good approach.
| @@ -279,18 +284,18 @@ def combine_curves( | |||
| ) | |||
|
|
|||
|
|
|||
| def orso_datasets_from_measurements( | |||
| def from_measurements( | |||
There was a problem hiding this comment.
Can we think of a better name? I don't have great suggestions, I thought of something like batch_reduction, but it's not super descriptive either...
| # Check if any of the targets need ReducibleData if | ||
| # ReflectivityOverQ already exists. | ||
| # If they don't, we can avoid recomputing ReducibleData. | ||
| targets = target if hasattr(target, '__len__') else (target,) |
There was a problem hiding this comment.
Can there ever be a danger that someone passed a string as target and the __len__ check fails? I guess if they passed a string, they will have other problems earlier? (target not found in graph)
This PR implements a shortcut through the workflow that is useful in several common scenarios involving multiple samples, angles and potentially multiple runs per angle.