-
-
Notifications
You must be signed in to change notification settings - Fork 9
Fix in normalize and normalize per image for float32 #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR adds immutability tests for all normalize and normalize_per_image variants, updates normalization functions to operate on copies and return new float32 arrays, tightens per-image clipping to the [0,1] range, and bumps the package version. Class diagram for updated normalization functionsclassDiagram
class normalize_numpy {
+np.ndarray normalize_numpy(img: np.ndarray, mean: float | np.ndarray, denominator: float | np.ndarray)
Note: Now always returns a new float32 array (copy=True)
}
class normalize_per_image_numpy {
+np.ndarray normalize_per_image_numpy(img: np.ndarray, normalization: str, eps: float)
Note: Now clips output to [0, 1] for min_max and min_max_per_channel
}
class normalize_per_image_lut {
+np.ndarray normalize_per_image_lut(img: np.ndarray, normalization: str, eps: float, max_value: int)
Note: Now clips LUT output to [0, 1] for min_max and min_max_per_channel
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if dtype == np.uint8: | ||
| original_img = np.random.randint(0, 256, size=shape, dtype=dtype) | ||
| else: | ||
| original_img = np.random.randn(*shape).astype(dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
=======================================
Coverage ? 93.96%
=======================================
Files ? 17
Lines ? 1921
Branches ? 0
=======================================
Hits ? 1805
Misses ? 116
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures normalization functions do not modify their inputs, updates clipping behaviour for float-based normalization, and bumps the package version.
- Added tests to confirm
normalize*routines preserve the original image - Changed
normalize_numpyand related functions to always copy arrays and adjust clipping to [0,1] for min-max - Bumped version from 0.0.32 to 0.0.33
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_normalize_per_image.py | Added test_normalize_per_image_preserves_original to cover all variants |
| tests/test_normalize.py | Added test_normalize_preserves_original_image for main normalization |
| pyproject.toml | Incremented project version to 0.0.33 |
| albucore/functions.py | Updated copy semantics in normalize_numpy, adjusted min-max clipping |
Comments suppressed due to low confidence (2)
albucore/functions.py:284
- [nitpick] astype(copy=True) always allocates a new array; consider using copy=False or restructuring the computation to minimize unnecessary copies and reduce memory pressure.
img = img.astype(np.float32, copy=True)
albucore/functions.py:291
- [nitpick] Calling astype(copy=True) here produces an extra array even when img is already float32; consider avoiding redundant copies by using copy=False or performing the operation in-place.
return (img * denominator).astype(np.float32, copy=True)
Summary by Sourcery
Preserve input immutability and correct output range in normalization routines, extend tests accordingly, and bump the package version.
Bug Fixes:
Enhancements:
Tests:
Chores: