-
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
Refactor to handle wavelength bands during normalization step #68
Conversation
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
src/esssans/i_of_q.py
Outdated
out = sc.concat(sections, band_dim) | ||
out.coords[dim] = wavelength_bands | ||
return out | ||
q_all_pixels = data_q.bins.concat(dims_to_reduce) |
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.
Now that the body of this function is so small, and the one for the dense merging is also much reduced, would it make more sense to just inline the code in merge_spectra
?
I don't mind, it's a matter of taste. It may still be more readable as it is now.
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.
Done
src/esssans/i_of_q.py
Outdated
del stripped.coords[name] | ||
to_flatten = [dim for dim in data_q.dims if dim in dims_to_reduce] | ||
|
||
dummy_dim = str(uuid4()) | ||
# Make sure that dims to flatten are contiguous, and that Q is the last dim | ||
# Make dims to flatten contiguous, keep wavelength is the last dim |
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.
# Make dims to flatten contiguous, keep wavelength is the last dim | |
# Make dims to flatten contiguous, keep wavelength as the last dim |
Aside from simplifying code, this has the advantage of offering more flexibility. For example, it will allow for running the direct-beam iteration in the pre-computed
CleanSummedQ
, with minimal cost per iteration, i.e., an overall speedup.This would also allow for pre-computing a modified denominator term containing no monitors. Monitors could then be factor in in the final step. This would also reduce the impact of variances of monitor broadcasting, since a broadcast to all pixel is avoided (only broadcast to all Q). We can explore this in a follow-up.