Small fixes to DiskChopper.from_nexus

[nvaytet]

• Use rotation_speed_setpoint if available (reducing amount of parsing/filtering needed)
• Handle DataArray fields (would raise if type was not variable, even though the type hints claimed that the function accepts variable and data array)
• Remove unused function
• Add method to convert DiskChopper to a dict, which makes it easy to create a DataGroup from it

[jl-wynen]

So do we trust that whenever there is a setpoint in the file, the rotation speed is constant?

[nvaytet]

I guess it depends what we want to use a DiskChopper for.
If we want to use the logs to filter out events where the choppers were out of phase or something like that, then we want to keep the entire log.
If we want to know thw chopper parameters (e.g. find which tof LUT we want to load) then we just want static parameters.

I had assumed the use case was the latter but maybe that was wrong.
How do we support both in a good way?
Do we need to construct the DiskChopper if we are doing event filtering, or can we do it by just looking at the NXlogs?

[jl-wynen]

I think it comes down to the domain types in a workflow. DiskChopper is the actual implementation which should support both use cases. But the workflow has to ensure that the conversion from RawChoppers to DiskChoppers does the right thing. We could clarify this by using a type like DiskChopperParameters which uses the setpoint from RawChoppers. And we can have DiskChopperLogs to contain time series for event filtering.

[nvaytet]

DiskChopper is the actual implementation which should support both use cases.

But in 'Build DiskChopper', you are not keeping the logs, you are extracting a meaningful frequency from a log a frequencies.

So the DiskChopper doesn't support both, it just wants a single value (it even raises an error if the variable has more than 0 dimensions).

I guess that's probably what you meant by

But the workflow has to ensure that the conversion from RawChoppers to DiskChoppers does the right thing.

Instead of having an automatic conversion where we take the setpoint if available, we leave it to individual workflows to decide how they want to convert from RawChoppers?

I magined comparing chopper setting with the ones in the lookup table would be part of the GenericTofWorkflow. This would mean that each technique would have to patch the generic workflow with a provider that converts the RawChoppers?

[jl-wynen]

I can be part of the generic workflow. We can have a provider that uses the setpoint by default but can be replaced when necessary.

But in 'Build DiskChopper', you are not keeping the logs, you are extracting a meaningful frequency from a log a frequencies.

I wrote the docs long before we had the lut based approach. So this may need to be changed.

So the DiskChopper doesn't support both, it just wants a single value (it even raises an error if the variable has more than 0 dimensions).

Right, then we should use the data group (RawChoppers) for filtering if that is enough. Or we change DiskChopper to be more flexible.

[nvaytet]

Right, then we should use the data group (RawChoppers) for filtering if that is enough.

Fine by me. This PR was not addressing filtering, it just wants to construct a chopper from a nexus group.
If a setpoint is present in the group, we use that. If it isn't, we use the rotation_speed and assume/hope that some pre-processing was done beforehand so that it is now a scalar value instead of a log.

If it isn't a scalar, the constructor raises.
Also, maybe _get_1d_variable should be renamed to _get_0d_variable?

[jl-wynen]

I agree that filtering is out of scope here. But I don't like a function called from_nexus to decide internally what data to use. I would expect it to only convert the input data to a new representation. Also, I don't like that it decides whether to use a measured value or a user input. We should be explicit about what we use.
Can you add a new function or rename this one to something like from_nexus_setpoint? And then only look at the setpoint instead of deciding automatically?

Also, maybe _get_1d_variable should be renamed to _get_0d_variable?

Makes sense.

[nvaytet]

Can you add a new function or rename this one to something like from_nexus_setpoint? And then only look at the setpoint instead of deciding automatically?

I could. But I also don't like the method that is called from_nexus that actually can't read in a nexus group, the rotation speed and the phase need to be pre-processed.

[nvaytet]

Conclusion from in-person discussion: we leave the rotation_speed as is for now (we don't use the setpoint), until we have more info from ECDC. We may need to do some processing for the phase also as there will not be a phase_setpoint. So we may need a more automatic way of making choppers from nexus logs.

I will rename _get_1d_variable to _get_0d_variable.

[jl-wynen]

The changes here look good. Just one more change missing in the docs.

[jl-wynen]

Can you update the notebook in the docs to reflect this?

[nvaytet]

See update