perf(illumination): vectorize supported batch paths - #424
Conversation
Reviewer's GuideVectorizes Illumination’s batch path by reusing a shared gradient and selectively applying whole-batch multiplication and clipping, adds empty-batch handling, and extends ASV benchmark and coverage mappings so Illumination’s batch performance contracts are exercised via both Compose and direct routes. Flow diagram for vectorized Illumination.apply_to_images batch pathflowchart TD
A[apply_to_images] --> B{"mode == linear"}
B -- yes --> C[fpixel.apply_linear_illumination_batch]
B -- no --> D{"images.shape[0] == 0"}
D -- yes --> E[return images.copy]
D -- no --> F[fpixel.create_illumination_gradient]
F --> G[set clip_required]
G --> H{"images.dtype == np.uint8<br/>or<br/>images.shape[-1] <= 4 and clip_required"}
H -- yes --> I[_apply_to_batch_same_shape with albucore.multiply_by_array]
H -- no --> J[albucore.multiply_by_array on images]
I --> K[set result]
J --> K
K --> L{"images.dtype == np.float32<br/>and clip_required"}
L -- yes --> M[albucore.clip result inplace]
L -- no --> N[return result]
M --> O[return clipped result]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The float32 parity test for batch vs per-image illumination uses
assert_array_equal, which may be brittle for future numeric changes; considerassert_allclosewith a tight tolerance instead for the floating-point cases. - The new benchmark coverage assertions for Illumination rely on hardcoded case IDs and a fixed
coveredcount of 12; you might factor out helpers to derive these expectations from the scenario matrix to reduce maintenance when cases change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The float32 parity test for batch vs per-image illumination uses `assert_array_equal`, which may be brittle for future numeric changes; consider `assert_allclose` with a tight tolerance instead for the floating-point cases.
- The new benchmark coverage assertions for Illumination rely on hardcoded case IDs and a fixed `covered` count of 12; you might factor out helpers to derive these expectations from the scenario matrix to reduce maintenance when cases change.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Sorry @aswanth-07, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
4de6a45 to
c2dcfc1
Compare
|
The only failed check is the Windows/Python 3.12 compatibility shard, which stopped during environment setup because |
Fixes #53.
Summary
apply_to_imagesandCompose(images=...)across all requested modes, sizes, channels, dtypes, and batch sizesCorrectness
The focused parity matrix compares every batch result with the corresponding single-image kernel for:
linear,corner, andgaussianuint8andfloat32It also covers empty batches in all three modes. The same gradient is created once and reused across the batch.
While expanding the parity matrix, I found that current
maindoes not clip float32 corner/Gaussian batch results in the cases whose multiplier can exceed 1, although the single-image kernels are decorated with@clipped. This patch clips those batch results so the public batch route is bit-exact with the single-image contract.Performance coverage
The ASV matrix now measures both direct and Compose routes for 256, 512, and 1024 square images; 1, 3, and 5 channels;
uint8andfloat32; all three modes; and batch sizes 2, 4, 8, and 16. The largest cells are intentionally left to the repository's scheduled or manual ASV workflow rather than being run on my Windows laptop.A safe one-thread local sample at 256x256, batch size 4 showed the useful float32 native paths improving representative cells by roughly 7-28%, while the guarded uint8 routes remained effectively unchanged. Clipped 1/3-channel float32 stays on per-image multiplication because whole-batch multiplication plus the required clip regressed; 5-channel float32 still improved by roughly 15%.
Validation
python -m pytest tests/test_transforms.py -q -k illumination- 51 passedpython -m pytest tests/test_benchmark_coverage.py tests/test_select_benchmark_filters.py -q- 28 passedAI assistance
OpenAI Codex assisted with repository analysis, implementation, tests, benchmarks, and drafting this pull request. I reviewed the resulting diff and validation output before submission.
Summary by Sourcery
Vectorize Illumination batch processing while aligning batch behavior and benchmark coverage with single-image contracts and performance requirements.
New Features:
Bug Fixes:
Enhancements:
Tests: