-
Notifications
You must be signed in to change notification settings - Fork 1
Begin adding Zoom sample workflow #94
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
Conversation
docs/user-guide/zoom.ipynb
Outdated
| "# TODO Figure out the filetypes we have as inputs. Can we not use the workflow\n", | ||
| "# from ESSsans? If the problem is histogrammed files, can we add support there?\n", |
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.
Note this. Why are the polarized files different from what we use in ESSsans?
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.
Some test files are on Water:
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.
Sorry, the file is too big...Simon, can you upload the ones I have sent in slack?
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.
Note this. Why are the polarized files different from what we use in ESSsans?
Different monitors might have been for background measurements. Also, it is saved with four different spin channels, like the 3He measurements, with 4 periods
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.
We need to speak of how to do that for the files at ESS, I think we had a previous post on that. I remeber "File storage and data handling #43"
aec5f26 to
8d74542
Compare
astellhorn
left a comment
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.
The workflow itself works, but I have tested it for the incoming unpolarized state, so the notebook has to be changed a bit (using incomping_polarized=False). Could we upload two different zoom.ipynb files, one for "incoming_polarized" and one for "incoming_unpolarized" (I think I left this comment below also)? If that is ok with you I can save the two notebooks and try to push it in github, but I remember that last time I wanted to push this kind of additional fileupload, you anyhow had to do it yourself - should I upload instead the two notebooks in the polarization channel? I really would like to have two different notebooks for the tests, so that I dont get confused when I try loading the sample data for the next tests.
| "id": "32", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "The sample runs are event data, so we can simply merge all runs to obtain a single event-mode $I(Q)$.\n", |
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.
Johannes and me checked that sans.with_sample_runs is based on WavelengthScaledQ and I can therefore not use it for the computation of IofQxy (instead of IofQ), as for that WavelengthScaledQxy would be needed. Can that be inserted?
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 opened scipp/esssans#220 so this can be addressed.
|
Another change for the notebook: for cell 12 computing wav_min = 1.75 * sc.Unit('angstrom')
wav_max = 16.5 * sc.Unit('angstrom')
transmission_truncated = raw_transmission['wavelength', wav_min:wav_max]
transmission_depolarized = transmission_truncated['time', -1].copy()
transmission = transmission_truncated['time', :-1].copy()--> I didnt leave this comment in the review as it was nothing that changed in this branch compared to before, but I now have used |
|
@astellhorn Sorry I have not had time to look into your answers as there is some ongoing prep work for ICNS on Thursday. Will be on summer holiday soon after for 3 weeks. Do you depend on updates here before August? |
I think we can catch up in August for this! Have a nice holidays |
ebe5771 to
b4ca601
Compare
|
@astellhorn I think I addressed everything I can now. If you want to try it out again, you need scipp/esssans#221. There are more TODOs left, but maybe we can just get this merged in and address the rest in a follow-up, since there are too many discussion threads here already. |
|
I will merge this, remaining changes will be addressed on follow-ups. |
This is incomplete since I do not have access to a file with a sample. Just for reference.Needs scipp/esssans#214 (branch
isis-histogram-files) if you want to run the Zoom notebook (not on CI, since it needs Mantid an non-public files).The notebook is work-in-progress (see many to-dos, which will not be resolved in this PR, I think), but other changes should be reviewable.