Repository navigation
fix(algorithms): consolidate makeuint8/makeuint16 into a shared helper - #138
Merged
Merged
Conversation
makeuint8 and makeuint16 duplicated the same intensity-rescaling logic end to end, differing only in the target integer class. Extract that logic into a new private helper, stack.normalizeIntensity, parameterized by target class, and make both public functions thin wrappers around it with arguments blocks and full help text. Consolidating the two implementations surfaced four defects that were present in the prior code: - makeuint16 cast its result with `uint8(...)` instead of `uint16(...)`, returning uint8-classed data from a function whose entire purpose is to produce uint16 output. This looks like a copy-paste-from-makeuint8 artifact: the docstring, the 2^16 scale factor, and even a "[ ] Combine with makeuint8" to-do comment were updated for a 16-bit function, but this one line was not. The only caller in the codebase, MotionCorrection's `dataTypeOut == "uint16"` export path, received uint8-classed, almost entirely saturated data instead of properly scaled uint16 output whenever a user configured 16-bit motion- correction export with brightness recasting enabled. Verified against realistic 16-bit-range input: 100% of output pixels saturated to 255, collapsing the corrected stack to a single constant value. - makeuint16 additionally scaled with `normalized .* 2^16 - 1`. Operator precedence makes `.*` bind tighter than binary `-`, so this computed `(normalized .* 2^16) - 1` rather than the intended `normalized .* (2^16 - 1)`, an off-by-about-one error across most of the output range, independent of the class-cast defect above. The shared helper scales by intmax(targetClass) instead, which removes the magic number and the ambiguity for both classes. - The float-class guard `~isa(x,'single') || ~isa(x,'double')` is a tautology: by De Morgan's law it evaluates true for every class, so double-precision input was unconditionally downcast to single before rescaling, even though the code's intent was clearly to skip that cast for inputs already in a floating-point class. - The tolerance default was applied whenever bLims was empty, because the guard checked `isempty(bLims)` instead of `isempty(tolerance)`. Any tolerance a caller supplied was silently discarded and replaced with 0.0005 whenever bLims was not also given. The 1x2xn bLims convention documented for 4-D, multi-color-channel input was already broken before this change (confirmed against the prior implementation with the same input). It remains unfixed here: no caller in the codebase exercises it, and correcting it needs a confirmed contract for which dimension holds color, which is not available from the current code or callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers the shared rescaling behavior of both functions in one parameterized test class: explicit vs. estimated limits, saturation outside bLims, output size/class, NaN exclusion during limit estimation, cropAmount, and argument validation. Also adds regression tests for the two behavioral bugs fixed in the prior commit: a custom tolerance now actually changes the estimated limits instead of being silently discarded, and makeuint16's midpoint output now lands exactly on the correct value instead of the operator-precedence error's off-by-about-one result. A further test checks makeuint8 and makeuint16 agree on relative scaling now that they share one implementation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Test Results333 tests 333 ✅ 30s ⏱️ Results for commit b61b835. |
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
1 participant
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.
Consolidates
stack.makeuint8andstack.makeuint16, which duplicated the same intensity-rescaling logic, into a shared private helper,normalizeIntensity. Both public functions are now thin wrappers withargumentsblocks and full help text. Adds a parameterized unit test class covering both.Four bugs in the original code are fixed along the way:
makeuint16cast its output touint8, so its only caller, theuint16export path inMotionCorrection, received saturated 8-bit data instead of 16-bit output.makeuint16scaled by2^16-1without parentheses, so operator precedence produced an off-by-one error across most of the range.~isa(x,'single') || ~isa(x,'double')is always true, so double input was always downcast to single.tolerancewas silently discarded wheneverbLimswas empty, because the guard checked the wrong variable.Details on each are in the fix commit message.
The documented
1x2xnbLimsform for 4-D multi-channel input was already broken before this change and is left as is, since nothing in the codebase calls it.🤖 Generated with Claude Code