[WIP] Improve downsampling performance #1406
Conversation
Summary of ChangesHello @daniel-wer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant performance improvement for downsampling operations within the system. The changes primarily involve integrating Numba for just-in-time compilation of critical data processing functions, adjusting buffer sizes to handle larger data volumes more efficiently, and refining the chunking strategy to optimize data access patterns. These enhancements are designed to reduce the time and resources required to generate lower-resolution representations of datasets. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request significantly improves downsampling performance by introducing Numba-jitted mode calculation and refining chunking strategies. The change to exclusively use zarr3 for opening files also streamlines the process. However, there are a few areas that warrant further review to ensure correctness and optimal performance, particularly regarding the zarr fallback, the fast_mode implementation, memory implications of increased buffer size, and the chunk shape calculations for zipped chunks.
| return tensorstore.open( | ||
| {"driver": "zarr3", "kvstore": {"driver": "file", "path": str(path)}} | ||
| ).result() |
There was a problem hiding this comment.
The previous implementation of _try_open_zarr attempted to open with zarr3 first and then fell back to zarr (v2) if zarr3 failed. The current change removes this fallback, exclusively trying zarr3. If the system is expected to handle older zarr (v2) datasets, this change could lead to TensorStoreError for those files. Please confirm if zarr (v2) support is intentionally being dropped or if a fallback mechanism is still required.
| target_chunk_shape = Vec3Int([1024, 1024, 512]).pairmax( | ||
| target_view.info.shard_shape | ||
| ) | ||
| source_view.for_zipped_chunks( | ||
| # this view is restricted to the bounding box specified in the properties | ||
| func, | ||
| target_view=target_view, | ||
| executor=executor, | ||
| source_chunk_shape=target_chunk_shape * target_mag.to_np(), | ||
| target_chunk_shape=target_chunk_shape * target_mag.to_np(), |
There was a problem hiding this comment.
In downsample_mag, both source_chunk_shape and target_chunk_shape are calculated using target_chunk_shape * target_mag.to_np(). This implies that the chunk shapes for both the source and target views are identical, scaled by the target magnification. This might be incorrect. source_chunk_shape should typically be derived from prev_mag_view.info.shard_shape and from_mag, while target_chunk_shape should be derived from target_view.info.shard_shape and target_mag. Using the same scaled chunk shape for both could lead to inefficient or incorrect chunking when reading from the source or writing to the target, especially if from_mag and target_mag are different.
| @numba.jit(nopython=True, nogil=True) | ||
| def fast_mode(input_array: np.ndarray) -> np.ndarray: | ||
| values = np.zeros(input_array.shape[0], dtype=input_array.dtype) | ||
| counter = np.zeros(input_array.shape[0], dtype=np.uint8) | ||
| output_array = np.zeros(input_array.shape[1], dtype=input_array.dtype) | ||
| for row_index in range(input_array.shape[1]): | ||
| values[0] = input_array[0, row_index] | ||
| counter[:] = 0 | ||
| value_offset = 1 | ||
| for col_index in range(1, input_array.shape[0]): | ||
| value = input_array[col_index, row_index] | ||
| found_value = False | ||
| for i in range(col_index): # iterate one less | ||
| if value == values[i]: | ||
| counter[i] = counter[i] + 1 | ||
| found_value = True | ||
| break | ||
| if not found_value: | ||
| values[value_offset] = value | ||
| value_offset += 1 | ||
| mode = values[np.argmax(counter)] | ||
| output_array[row_index] = mode | ||
|
|
||
| return output_array |
There was a problem hiding this comment.
The fast_mode function, while Numba-jitted, uses nested Python-style loops for finding the mode. For larger arrays, this pattern can still be less efficient than vectorized NumPy operations, even with Numba. Consider if a more idiomatic Numba/NumPy approach, such as using np.unique with return_counts and np.argmax, could offer better performance and readability, especially if the number of unique values is small compared to input_array.shape[0].
| # data that is read is up to 1024³ vx in the source magnification. Using larger | ||
| # shapes uses a lot of RAM, especially for segmentation layers which use the mode filter. | ||
| # See https://scm.slack.com/archives/CMBMU5684/p1749771929954699 for more context. | ||
| return Vec3Int.full(256).pairmin(array_info.shard_shape) | ||
| return Vec3Int.full(512).pairmin(array_info.shard_shape) |
There was a problem hiding this comment.
The determine_downsample_buffer_shape function now sets the buffer size to 512^3 vx (from 256^3 vx). While this can improve performance by processing larger blocks, it also significantly increases memory consumption, especially for segmentation layers using the mode filter. Please ensure that this increased memory usage is acceptable for typical operating environments and data sizes.
| shape = (num_channels,) + target_bbox_in_mag.size.to_tuple() | ||
| shape_xyz = target_bbox_in_mag.size_xyz | ||
| file_buffer = np.zeros(shape, target_view.get_dtype()) | ||
| file_buffer = np.zeros(shape, target_view.get_dtype(), order="F") |
There was a problem hiding this comment.
Specifying order="F" (Fortran-contiguous) for np.zeros can be a good optimization if subsequent operations on file_buffer primarily access data column-wise or along the last axis first. However, if the data is mostly accessed row-wise or along the first axis, order="C" (C-contiguous, default) might be more efficient. Please confirm that Fortran-contiguous order aligns with the primary access patterns for file_buffer to ensure optimal performance.
Description:
Issues:
Todos:
Make sure to delete unnecessary points or to check all before merging: