-
Notifications
You must be signed in to change notification settings - Fork 2
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
Save results to NXcanSAS #32
Conversation
To prevent accidental modification
|
||
providers = conversions.providers + i_of_q.providers + normalization.providers | ||
providers = ( |
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 changed providers to tuples everywhere in order to prevent accidental modifications by users. It is tempting to use code like sans.providers.append
. But this leads to type errors because Sciline wants providers to be passed as a list. IMHO, this should be changed in Sciline, it should accept any sequence.
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.
Looks good, two small comments.
src/esssans/metadata.py
Outdated
def run_number(raw_data: RawData[SampleRun]) -> RunNumber: | ||
"""Get the run number from the raw sample data.""" | ||
return RunNumber(int(raw_data['run_number'])) | ||
|
||
|
||
def run_title(raw_data: RawData[SampleRun]) -> RunTitle: | ||
"""Get the run title from the raw sample data.""" | ||
return RunTitle(raw_data['run_title'].value) |
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 think those names a Mantid-specific, so maybe this should move into sans2d
. NeXus has this information as well, but probably under slightly different names (I think title
and entry_identifier
in NXentry).
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.
Good point. I'll move it.
|
||
def save_background_subtracted_iofq( | ||
*, | ||
iofq: BackgroundSubtractedIofQ, |
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.
Do we ever need to store the result without background subtraction?
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.
Don't know. Given how Sciline works, we would need two separate functions. We can easily add another one if we need to.
Fixes #15