Support coords with different dims #286
Conversation
d3e340d to
833fd0d
Compare
SimonHeybrock
left a comment
There was a problem hiding this comment.
Looks ok, but more detailed comments would make the rather involved logic/branching easier to follow.
| monitor: sc.DataArray, | ||
| dim: str, | ||
| *, | ||
| broadcast_to: sc.DataArray | None = None, |
There was a problem hiding this comment.
Completely unrelated, but I really feel that having a better way of type-hinting details about DataArray (binned? dims? coords?) would make code much easier to read.
| ) | ||
| return detector / norm | ||
|
|
||
| case _HistogramNormalizationMode.BinsDifferentDim: |
There was a problem hiding this comment.
Some comments here or clearer enum names would be really helpful. Took me a while to understand that this means "Histogram with (N-D) wavelength coord but not wavelength dim".
src/ess/reduce/normalization.py
Outdated
| return _HistogramNormalizationMode.BinsDifferentDim | ||
|
|
||
|
|
||
| def _coord_midpoints(da: sc.DataArray, name: str) -> sc.Variable: |
There was a problem hiding this comment.
So it is not technically computing coord midpoints, but rather converting to bin-centers? Would be slightly less confusing when reading the code higher up.
| *, | ||
| monitor: sc.DataArray, | ||
| uncertainty_broadcast_mode: UncertaintyBroadcastMode, | ||
| skip_range_check: bool = False, |
There was a problem hiding this comment.
I cannot see where this param is used, none of the providers seems to set it?
There was a problem hiding this comment.
There are no providers for this in essreduce. But see scipp/essdiffraction#216
There was a problem hiding this comment.
Right. Can you clarify why you chose to "mask and skip check" there instead of masking here?
There was a problem hiding this comment.
I expanded the docstring. Is this clear enough?
fda9845 to
5eec2df
Compare
Pull request was converted to draft
779797f to
b99e8d8
Compare
|
@nvaytet This seems to build now with a single threaded build. Should we merge it as is? Or do you want me to debug this further? |
|
Go ahead an merge 👍 |
Fixes #283