-
Notifications
You must be signed in to change notification settings - Fork 2
Performance #71
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
Performance #71
Changes from all commits
63a90d5
7ebbba3
011e791
e629e44
0f8bf95
117bc56
0901d8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,8 +102,10 @@ def _group(detectors: Dict[str, sc.DataArray]) -> Dict[str, sc.DataGroup]: | |
| def group(key: str, da: sc.DataArray) -> sc.DataArray: | ||
| if key in ["high_resolution", "sans"]: | ||
| # Only the HR and SANS detectors have sectors. | ||
| return da.group("sector", *elements) | ||
| res = da.group(*elements) | ||
| res = da.group("sector", *elements) | ||
| else: | ||
| res = da.group(*elements) | ||
| res.coords['position'] = res.bins.coords.pop('position').bins.mean() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you check whether this changes the final result?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was hoping there are tests for that...?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are no tests for the complete The problem is that the workflow is incomplete. Most updates in the near future will break regression tests. |
||
| res.bins.coords.pop("sector", None) | ||
| return res | ||
|
|
||
|
|
@@ -167,15 +169,11 @@ def _extract_detector( | |
| return events | ||
|
|
||
|
|
||
| def get_source_position( | ||
| raw_source: RawSource[RunType], | ||
| ) -> SourcePosition[RunType]: | ||
| def get_source_position(raw_source: RawSource[RunType]) -> SourcePosition[RunType]: | ||
| return SourcePosition[RunType](raw_source["position"]) | ||
|
|
||
|
|
||
| def get_sample_position( | ||
| raw_sample: RawSample[RunType], | ||
| ) -> SamplePosition[RunType]: | ||
| def get_sample_position(raw_sample: RawSample[RunType]) -> SamplePosition[RunType]: | ||
| return SamplePosition[RunType](raw_sample["position"]) | ||
|
|
||
|
|
||
|
|
@@ -184,10 +182,11 @@ def patch_detector_data( | |
| source_position: SourcePosition[RunType], | ||
| sample_position: SamplePosition[RunType], | ||
| ) -> ReducibleDetectorData[RunType]: | ||
| out = detector_data.copy(deep=False) | ||
| out.coords["source_position"] = source_position | ||
| out.coords["sample_position"] = sample_position | ||
| return ReducibleDetectorData[RunType](out) | ||
| return ReducibleDetectorData[RunType]( | ||
| detector_data.assign_coords( | ||
| source_position=source_position, sample_position=sample_position | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def geant4_detector_dimensions( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |||||||||||||||||
| # Copyright (c) 2023 Scipp contributors (https://github.com/scipp) | ||||||||||||||||||
| """Grouping and merging of pixels / voxels.""" | ||||||||||||||||||
|
|
||||||||||||||||||
| import scipp as sc | ||||||||||||||||||
|
|
||||||||||||||||||
| from .types import ( | ||||||||||||||||||
| DspacingBins, | ||||||||||||||||||
| DspacingData, | ||||||||||||||||||
|
|
@@ -12,23 +14,41 @@ | |||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def _drop_grouping_and_bin( | ||||||||||||||||||
| data: sc.DataArray, *, dims_to_reduce: tuple[str, ...] | None = None, edges: dict | ||||||||||||||||||
| ) -> sc.DataArray: | ||||||||||||||||||
| all_pixels = data if dims_to_reduce == () else data.bins.concat(dims_to_reduce) | ||||||||||||||||||
|
Comment on lines
+17
to
+20
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
because
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was using it for the two-theta case, but then refactored. Since I wanted to consider moving this to ESSreduce I thought I'd keep it. |
||||||||||||||||||
| # all_pixels may just have a single bin now, which currently yields | ||||||||||||||||||
| # inferior performance when binning (no/bad multi-threading?). | ||||||||||||||||||
| # We operate on the content buffer for better multi-threaded performance. | ||||||||||||||||||
| if all_pixels.ndim == 0: | ||||||||||||||||||
| return ( | ||||||||||||||||||
| all_pixels.value.bin(**edges) | ||||||||||||||||||
| .assign_coords(all_pixels.coords) | ||||||||||||||||||
| .assign_masks(all_pixels.masks) | ||||||||||||||||||
| ) | ||||||||||||||||||
| else: | ||||||||||||||||||
| return all_pixels.bin(**edges) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def focus_data_dspacing( | ||||||||||||||||||
| data: DspacingData[RunType], | ||||||||||||||||||
| dspacing_bins: DspacingBins, | ||||||||||||||||||
| data: DspacingData[RunType], dspacing_bins: DspacingBins | ||||||||||||||||||
| ) -> FocussedDataDspacing[RunType]: | ||||||||||||||||||
| out = data.bins.concat().bin({dspacing_bins.dim: dspacing_bins}) | ||||||||||||||||||
| return FocussedDataDspacing[RunType](out) | ||||||||||||||||||
| return FocussedDataDspacing[RunType]( | ||||||||||||||||||
| _drop_grouping_and_bin(data, edges={dspacing_bins.dim: dspacing_bins}) | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def focus_data_dspacing_and_two_theta( | ||||||||||||||||||
| data: DspacingData[RunType], | ||||||||||||||||||
| dspacing_bins: DspacingBins, | ||||||||||||||||||
| twotheta_bins: TwoThetaBins, | ||||||||||||||||||
| ) -> FocussedDataDspacingTwoTheta[RunType]: | ||||||||||||||||||
| bins = {twotheta_bins.dim: twotheta_bins, dspacing_bins.dim: dspacing_bins} | ||||||||||||||||||
| if "two_theta" in data.bins.coords: | ||||||||||||||||||
| data = data.bins.concat() | ||||||||||||||||||
| return FocussedDataDspacingTwoTheta[RunType](data.bin(**bins)) | ||||||||||||||||||
| return FocussedDataDspacingTwoTheta[RunType]( | ||||||||||||||||||
| data.bin({twotheta_bins.dim: twotheta_bins}).bin( | ||||||||||||||||||
| {dspacing_bins.dim: dspacing_bins} | ||||||||||||||||||
| ) | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| providers = (focus_data_dspacing, focus_data_dspacing_and_two_theta) | ||||||||||||||||||
|
|
||||||||||||||||||
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.
Why the explicit limits? Does this really matter so much for performance?
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.
Because we have some voxels with NaN positions, and get an exception otherwise.