fix(image): correct block_size validation in PSNRB (and → or)#3367
Open
xodn348 wants to merge 1 commit intoLightning-AI:masterfrom
Open
fix(image): correct block_size validation in PSNRB (and → or)#3367xodn348 wants to merge 1 commit intoLightning-AI:masterfrom
xodn348 wants to merge 1 commit intoLightning-AI:masterfrom
Conversation
…ioWithBlockedEffect The `and` operator in the guard was logically incorrect: for any int input the first clause (`not isinstance(block_size, int)`) is False, which short- circuits the expression so the `< 1` range check is never reached. Zero and negative ints slip through silently, floats slip through when >= 1, and non-numeric types raise TypeError instead of the intended ValueError. Change `and` to `or` so that the check raises ValueError for every value that is not a positive integer, and add the same guard to the functional `peak_signal_noise_ratio_with_blocked_effect` which had no validation at all. Also fix a pre-existing RUF043 lint warning in the same test file (missing raw-string prefix on a regex match= pattern). Fixes Lightning-AI#3364
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
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.
Summary
PeakSignalNoiseRatioWithBlockedEffect.__init__contains a one-character logic error in its input guard: the condition usesandwhereoris required.With
and, the first clause (not isinstance(block_size, int)) evaluates toFalsefor everyint, which short-circuits the expression and skips the< 1check entirely. As a result:block_size=0andblock_size=-5are silently accepted instead of raisingValueError.block_size=1.5is silently accepted (float>= 1makes the second clauseFalse).block_size="foo"raisesTypeErrorfrom the comparison instead of the intendedValueError.This PR applies the same
or-based guard to the functionalpeak_signal_noise_ratio_with_blocked_effect, which had noblock_sizevalidation at all — an omission noted in the issue thread.Issue
Fixes #3364
Local verification
Risk
The change only affects the error-path for invalid
block_sizearguments — no functional computation is touched. Users passing a valid positive integer (block_size >= 1) are unaffected. The only observable behavioral change is that previously-ignored bad inputs now raiseValueErroras documented, which is the intended contract. No existing tests regress.📚 Documentation preview 📚: https://torchmetrics--3367.org.readthedocs.build/en/3367/