compression and one time/two time improvements - #105
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad, performance-critical changes (I/O format access, multiprocessing/threading, BLAS/Numba behavior) and needs careful human validation beyond the specific issues flagged.
Pull request overview
This PR focuses on speeding up pyCHX’s compressed-data and correlation pipelines by reducing repeated I/O and memory copies, adding mmap/indexed CMP frame access, and moving key reductions into Numba-compiled kernels while keeping the public API and numerical semantics consistent (within expected floating-point noise).
Changes:
- Add a new private performance module (
pyCHX/_performance.py) providing CPU/memory helpers plus Numba kernels for sparse scatter/sums, diagonal reductions, and two-time matrix finishing. - Rework compression to read Eiger data in larger contiguous blocks, stage/publish outputs atomically, and add indexed/mmap-backed CMP random access via
Multifile._raw_frame_view. - Optimize one-time/two-time correlation paths to reduce redundant work (block-based processing, cached intensities, BLAS/Numba thread limiting), and expand regression coverage/benchmarks.
File summaries
| File | Description |
|---|---|
| pyproject.toml | Adds required deps for new Numba + threadpool control based optimizations. |
| pyCHX/_performance.py | New private module with Numba kernels and CPU/memory helpers used by optimized paths. |
| pyCHX/Two_Time_Correlation_Function.py | Uses _performance diagonal reducers + thread limiting for faster one-time-from-two-time. |
| pyCHX/chx_correlationc.py | Two-time correlation now uses BLAS dsyrk + batched symmetric finishing; adds fast sparse scatter path and thread controls. |
| pyCHX/chx_correlationp.py | Refactors parallel g2 path to reduce I/O and rebalance ROI work, using _performance kernels. |
| pyCHX/chx_compress.py | Compression pipeline overhaul: staged/atomic publish, contiguous Eiger reads, persistent segment workers, mmap/indexed CMP reads. |
| pyCHX/chx_compress_analysis.py | Uses fused sparse scatter kernel for faster waterfall extraction. |
| pyCHX/tests/test_numerical_regressions.py | Adds many regression tests validating new kernels and semantics (with a few assertions needing tolerance fixes). |
| pyCHX/tests/test_import_compatibility.py | Ensures wildcard imports still bind optimized implementations. |
| pyCHX/tests/test_compression.py | Adds extensive compression + CMP layout/indexing validation tests. |
| pyCHX/benchmarks/init.py | Adds benchmarks package marker. |
| pyCHX/benchmarks/benchmark_xpcs.py | New opt-in benchmark CLI for measuring perf/memory/thread/process behavior. |
Review details
Suppressed comments (2)
pyCHX/tests/test_numerical_regressions.py:442
- This assertion compares floating-point arrays for exact equality; even with integer inputs, the correlation calculation produces float outputs and can vary by tiny rounding errors across platforms. Use assert_allclose instead to avoid flaky portable test failures.
np.testing.assert_array_equal(auto_two_Arrayc(integer_data, roi_mask), np.stack(integer_expected, axis=2))
pyCHX/tests/test_numerical_regressions.py:452
- Exact equality on floating-point results can be non-portable (different BLAS/CPU vectorization can change last-bit rounding). Use assert_allclose for this ROI-slice check as well.
np.testing.assert_array_equal(auto_two_Arrayc(data, roi_mask, index=5), expected_auto[:, :, 1:])
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| expected_auto = np.stack(expected_auto, axis=2) | ||
|
|
||
| np.testing.assert_allclose(auto_two_Arrayc(data, roi_mask), expected) | ||
| np.testing.assert_array_equal(auto_two_Arrayc(data, roi_mask), expected_auto) |
There was a problem hiding this comment.
🟡 Changes recommended
The parallel compression path in _compress_segment can mis-handle unbinned unsigned Eiger frames by casting before hot-pixel masking, which can change the compressed output for high-intensity/invalid pixel values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pyCHX/chx_compress.py:586
- Typo in user-facing log message: "seperated" should be "separated".
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
| image = np.asarray(source_image, dtype=dtype) | ||
| mask &= image < hot_pixel_threshold | ||
| flattened = image.ravel() | ||
| positions = np.flatnonzero((flattened > 0) & mask.ravel()) | ||
| values = flattened[positions] |
The main improvements come from reducing repeated I/O and memory copies, reusing workers more efficiently, and moving more numerical work into parallel Numba-compiled routines. Compression now reads Eiger data in larger contiguous blocks and accesses CMP data more directly through indexing and memory mapping, one-time correlations process all ROIs in a single pass through the data instead of rereading it per ROI, and the two-time correlations avoid redundant work by calculating only a triangle of the matrix and better parallelizing the reductions. So a significantly reduction of I/O, memory pressure, and multiprocessing overhead without changing the public API or expected numerical behavior beyond normal floating point differences
AI transparency: OpenAI's Codex assisted with this work