Vectorize numpy operations in structure splitting#592
Open
aymuos15 wants to merge 3 commits intobrainglobe:mainfrom
Open
Vectorize numpy operations in structure splitting#592aymuos15 wants to merge 3 commits intobrainglobe:mainfrom
aymuos15 wants to merge 3 commits intobrainglobe:mainfrom
Conversation
Author
|
I realise the codecov tests fail because of the tests. Should I patch this as well within this PR? #285 |
Member
|
Yes, please make sure all code is tested. |
Tests the vectorized masking logic that was refactored in structure splitting optimization. Ensures code coverage for the new vectorized array operations. partially attempting issue brainglobe#285.
for more information, see https://pre-commit.ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
What is this PR
Refactoring / performance improvement.
Why is this PR needed?
The post-processing in
split_cellsused element-by-element Python loops and a per-point helper function (check_centre_in_cuboid) instead of idiomatic vectorized numpy. This is predominantly slower and the patch is much cleaner.What does this PR do?
Replaces three loop/element-wise patterns in
split_cells()with vectorized numpy equivalents:relative_orig_centre— element-by-element subtraction →orig_centre - orig_cornercheck_centre_in_cuboidper point →np.all(..., axis=1)boolean maskabsolute_centres— per-column addition into a pre-allocated array →relative_centres + orig_cornerThe helper function
check_centre_in_cuboidis removed as its logic is now inlined.Benchmark results (median, 50 repeats)
Correctness verified: old and new outputs match (
np.allclose) at every size.References
N/A
How has this PR been tested?
test_underflow_issue_435exercisessplit_cellsend-to-end and continues to pass.np.allclosebetween old and new implementations across 5 input sizes.Is this a breaking change?
No. Pure internal refactor with identical outputs.
Does this PR require an update to the documentation?
No.
Checklist: