Read a scalar CLAHE clip_limit as a range, matching albumentations - #1350
Read a scalar CLAHE clip_limit as a range, matching albumentations#1350adhavan18 wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1350 +/- ##
=======================================
Coverage 86% 86%
=======================================
Files 114 114
Lines 14880 14886 +6
=======================================
+ Hits 12835 12841 +6
Misses 2045 2045 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes CLAHE scalar clip_limit handling so Kornia matches Albumentations.
Changes:
- Adds dedicated CLAHE range normalization.
- Adds scalar, pair, default, and backend-parity tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/rfdetr/datasets/kornia_transforms.py |
Normalizes scalar CLAHE limits to (1, value). |
tests/datasets/test_kornia_transforms.py |
Tests normalization and backend parity. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
all three addressed in the one-element sequenceconfirmed against the pinned albumentations before changing anything: so
the two test conventions
|
Albumentations applies to_tuple(clip_limit, low=1), so a scalar v means the range (1, v) and it samples from it. _as_range expands a scalar to the degenerate (v, v), so the Kornia path pinned every sample to maximum contrast enhancement while the CPU path varied it. 4.0 is the default on both sides, so this applied to the default configuration rather than only to unusual ones, and nothing was logged. A pair is already a range and is used as given, so only the scalar form changes. Introduced in roboflow#1277. Fixes roboflow#1349
A one-element sequence is not a scalar. Albumentations validates clip_limit as a float or an exact 2-tuple and raises on [4.0], while this helper read it through _as_range and turned it into (1.0, 4.0). That accepted a config the CPU backend refuses, which is the divergence the helper exists to remove. Also annotates the parametrized arguments and splits the cross-backend comparison into independent cases, so one failure no longer hides the rest.
e65d1e9 to
6d2fbfc
Compare
|
Bumping this since it's been quiet for a couple weeks — just rebased onto current develop, no conflicts. Happy to address anything if it needs another look. |
|
Hi, apologies for delay, I was off last week but I have on my radar for later this week... |
Fixes #1349.
The bug
Albumentations applies
to_tuple(clip_limit, low=1), so a scalarvmeans the range(1, v)and it samples from it per call._make_claherouted the value through_as_range, which expands a scalar to the degenerate(v, v).4.0is the default on both sides, so this applied with no user configuration at all:{}(default)(4.0, 4.0)(1.0, 4.0)(1.0, 4.0){"clip_limit": 4.0}(4.0, 4.0)(1.0, 4.0)(1.0, 4.0){"clip_limit": 2.0}(2.0, 2.0)(1.0, 2.0)(1.0, 2.0){"clip_limit": (2.0, 6.0)}(2.0, 6.0)(2.0, 6.0)(2.0, 6.0)Only the explicit-pair form agreed. Every scalar form, including the default, pinned the GPU path to maximum contrast enhancement on every sample while the CPU path varied it — a silent train/serve divergence with no warning, since from the code's point of view nothing was being collapsed.
This came in with #1277, which added the CLAHE mapping. Mine.
The fix
A dedicated
_as_clahe_clip_limitrather than a special case inside_as_range: the helper is correct about what it does and is used by transforms whose scalar semantics really are(v, v). Changing it there would have fixed CLAHE and broken the others.A pair is used as given, so only the scalar form changes.
Scope
I checked the other
_as_rangecall sites for the same class of mismatch:Sharpen(alpha) andGaussNoise(std_range) — albumentations rejects a scalar for both withValueError, so there is no scalar form to disagree about. Safe.GaussianBlur(sigma) — albumentations expands a scalarsigma_limitto(0, v), so the same gap exists, but only when a user passes a scalar; the configured default is already a pair. Left out of this PR deliberately — happy to fold it in here or do it separately, whichever you prefer.Verification
Nine assertions, all five configured forms plus direct parity against albumentations:
The three scalar cases fail on the parent commit:
test_clahe_maps_both_parametersstill passes —grid_sizeand the explicit pair are unaffected.ruff checkandruff format --checkclean on both files.pytestcannot collect the module in my environment (transformersis missingBackboneConfigMixin, unrelated to this diff), so I exercised the factory directly; CI here will be the better check.