Skip to content

Small fixes to DiskChopper.from_nexus #649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Nov 27, 2025
Merged

Small fixes to DiskChopper.from_nexus #649

Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
75e9485
use rotation setpoint if available, fix docs link, handle data array …
nvaytet Nov 12, 2025
4351b9d
add method to convert chopper to a dict, making it easy to create a D…
nvaytet Nov 12, 2025
431d180
Merge branch 'main' into chopper-from-nexus
nvaytet Nov 13, 2025
be3bbfd
remove use of setpoint and rename 1d to 0d
nvaytet Nov 25, 2025
391913d
Merge branch 'main' into chopper-from-nexus
nvaytet Nov 25, 2025
f38aaab
use setpoint, and delay if phase is not present
nvaytet Nov 25, 2025
3df3643
allow both rotation_speed and rotation_speed_setpoint
nvaytet Nov 25, 2025
d823a3c
add comment
nvaytet Nov 25, 2025
bd22186
add phase tests
nvaytet Nov 25, 2025
de80682
Merge branch 'main' into chopper-from-nexus
nvaytet Nov 25, 2025
01340d4
update processing choppers notebook in docs
nvaytet Nov 26, 2025
85f3d42
Merge branch 'main' into chopper-from-nexus
nvaytet Nov 26, 2025
95f8093
Merge branch 'main' into chopper-from-nexus
nvaytet Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/scippneutron/chopper/disk_chopper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,12 @@ def from_nexus(
)
return DiskChopper(
axle_position=chopper['position'],
frequency=_get_1d_variable(chopper, 'rotation_speed'),
frequency=_get_1d_variable(
chopper,
'rotation_speed_setpoint'
if 'rotation_speed_setpoint' in chopper
else 'rotation_speed',
Copy link
Member

@jl-wynen jl-wynen Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@nvaytet nvaytet Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jl-wynen jl-wynen Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nvaytet nvaytet Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jl-wynen jl-wynen Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nvaytet nvaytet Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jl-wynen jl-wynen Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nvaytet nvaytet Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nvaytet nvaytet Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
beam_position=_get_1d_variable(chopper, 'beam_position'),
phase=_get_1d_variable(chopper, 'phase'),
slit_height=chopper.get('slit_height'),
Expand Down Expand Up @@ -557,6 +562,10 @@ def _source_phase_factor(self, pulse_frequency: sc.Variable) -> int:
# of the slits, so use `max` here:
return round(max(quot.value, 1))

def as_dict(self) -> dict[str, Any]:
"""Return the DiskChopper fields as a dictionary."""
return dataclasses.asdict(self)


def _field_eq(a: Any, b: Any) -> bool:
if isinstance(a, sc.Variable | sc.DataArray):
Expand Down Expand Up @@ -627,12 +636,6 @@ def _get_edges_from_nexus(
}


def _len_or_1(x: sc.Variable) -> int:
if x.ndim == 0:
return 1
return len(x)


def _get_1d_variable(
dg: Mapping[str, sc.Variable | sc.DataArray], name: str
) -> sc.Variable:
Expand All @@ -642,9 +645,11 @@ def _get_1d_variable(
msg = (
"Chopper field '{name}' must be a scalar variable, {got}. "
"See the chopper user-guide for more information: "
"https://scipp.github.io/scippneutron/user-guide/chopper/pre-processing.html"
"https://scipp.github.io/scippneutron/user-guide/chopper/processing-nexus-choppers.html"
)

if isinstance(val, sc.DataArray):
val = val.data
if not isinstance(val, sc.Variable):
raise TypeError(msg.format(name=name, got=f'got a {type(val)}'))
if val.ndim != 0:
Expand Down
Loading