feat: more flexible helper for batch reduction

[jokasimr]

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.

[Copilot]

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.

[Copilot]

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.

Suggested change

	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):

[MridulS]

good bot!

[jokasimr]

🍬

[nvaytet]

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.

[jokasimr]

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.

[nvaytet]

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.

[nvaytet]

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...

[jokasimr]

Not sure either. I would like to avoid reduction in the name because I feel the term is already overloaded in our context, and strictly speaking you don't have to request a "reduced" data set here.

[nvaytet]

I asked copilot, and the suggestion was compute_reflectivity_datasets.
I guess it's because of the last thing in the docstring that says

    Returns
    ---------
    list of the computed ORSO datasets, containing one reflectivity curve each

which I guess should be changed if you can request other data than reflectivity curves?

[jokasimr]

Yes that's true, I'll change it.
By the way, I asked Nico earlier today and he's thinking about a better name, so we are probably going to have a suggestion from him soon.

[paracini]

We discussed some options and we came up with compute_pipeline_targets or get_workflow_targets. They both seem reasonably descriptive to me but maybe the shorter one is better

[nvaytet]

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)

[jokasimr]

I guess, but like you said, passing a string is illegal anyway and will lead to error at some point.

[jokasimr]

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.

Another problem with this approach is that the Angle is something we read from the file, so it seems backwards that the user has to know the angle before loading the file.

[jokasimr]

It would be good to merge this so we can get it out and receive some user feedback.
If it is the name that is holding this up I think we should just go with the current name and change it in the future if we come up with something better.
In that case the old name can be kept as an alias to the new, so backwards compatibility is not an issue.

[nvaytet]

Finding a good name is important I think, especially since this looks like one of the main functions that will be used all the time. You can of course keep the alias if you change it, as you say, but why not try to find something better now?
Why not ask @paracini or other users for input?

I would like to avoid reduction in the name because [...] you don't have to request a "reduced" data set here.

ok, but realistically, I would say reduced data will be requested over 90% of the time?