-
Notifications
You must be signed in to change notification settings - Fork 3
Migrate offspec #146
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.
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
Migrate offspec #146
Conversation
…nto migrate-offspec
src/ess/offspec/load.py
Outdated
graph: CoordTransformationGraph[ReferenceRun], | ||
) -> MonitorData[RunType]: | ||
full = sc.io.load_hdf5(filename) | ||
mon = full["monitors"]["monitor2"]["data"].transform_coords( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the monitor name a parameter. See, e.g., cell 5 in https://scipp.github.io/esssans/user-guide/isis/sans2d.html
@@ -0,0 +1,32 @@ | |||
# Copyright (c) 2025 Scipp contributors (https://github.com/scipp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the first line of the license header. Also in other files.
src/ess/offspec/data.py
Outdated
@@ -0,0 +1,38 @@ | |||
# SPDX-License-Identifier: BSD-3-Clause | |||
# Copyright (c) 2023 Scipp contributors (https://github.com/scipp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the year in all files.
src/ess/offspec/normalization.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the workflow has changed. The old one normalised in wavelength and then converted to Q. This one converts to Q first and then normalises. Did you check with the scientists that this makes sense? This workflow was meant to replicate the ISIS workflow for OFFSPEC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check with any scientist, but I'm sure it's not a problem, and I don't think we should bother them with this since it's a one-off workflow for an instrument at a different facility.
Maybe you want me to check with them anyway, but let me at least try to convince you that what we are doing here is fine.
This is how the measured intensities are modeled,
To estimate
In the above expression it's natural to think about it like we are integrating over
So doing the normalization in
src/ess/offspec/__init__.py
Outdated
) | ||
|
||
|
||
def OffspecWorkflow() -> sciline.Pipeline: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this into workflow.py
. There should be no definitions in __init__.py
, only imports.
Co-authored-by: Jan-Lukas Wynen <[email protected]>
…nto migrate-offspec
Fixes #32