Fix rescale() silent divide-by-zero on constant/single-element input - #78
Conversation
rescale(vec, from, upto) computed vec - min(vec), then divided by max(vec) of that shifted vector. For a constant vector (or any single-element vector), the shifted vector is all zeros, so max(vec) is 0, and dividing by it silently produces Inf/NaN for every element instead of an error or a meaningful value - no warning is raised. Fix: detect the zero-range case (max == min) up front and return the midpoint of the target range, (from + upto) / 2, for all non-NA elements instead. This matches the well-established convention used by scales::rescale() for the identical degenerate-input case (verified: scales::rescale(c(5,5,5), to = c(0,100)) returns c(50,50,50)) - using an existing, widely-used package's behavior as the precedent avoids guessing a novel convention for this edge case. Verified: normal (non-degenerate) input is unaffected; constant vectors and single-element vectors now return the range midpoint instead of all-NaN; NA elements are left as NA rather than being assigned the midpoint; a custom target range on constant input scales correctly too. Version bumped 2.8.16 -> 2.8.20 (distinct from sibling PRs #75/#76/#77, which also branch from the same 2.8.16 base and already claimed 2.8.17/2.8.18/2.8.19).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26ea755cad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The constant-input branch computed (from + upto) / 2, which can overflow to Inf before the division when from and upto are both large finite numbers (e.g. from = 1e308, upto = 1.1e308 - their sum exceeds .Machine$double.xmax, so the "midpoint" was Inf instead of the correct finite 1.05e308). Fix: compute the midpoint as from + (upto - from) / 2 instead - the subtraction of two same-magnitude numbers doesn't overflow, and adding a bounded increment to from doesn't either. Verified: the flagged overflow case now returns the correct finite midpoint (1.05e+308) instead of Inf; normal constant-input and non-degenerate cases are unchanged.
|
Could you please verify this pull request by Claude? Claude Code has been introducing many mistakes in recent pull requests, so I require a very critical review from you. Task 2 is that you resolve the branch conflicts so that I can merge the pull request. |
…zero # Conflicts: # DESCRIPTION # Development/config.R Co-authored-by: vertesy <5101911+vertesy@users.noreply.github.com>
I reviewed Claude's fix critically: Branch conflicts are resolved: merged |
The bug
rescale(vec, from, upto)computedvec - min(vec), then divided bymax(vec)of that shifted vector. For a constant vector (or any single-element vector), the shifted vector is all zeros, somax(vec)is0, and dividing by it silently producesInf/NaNfor every element instead of an error or a meaningful value — no warning is raised.Fix
Detect the zero-range case (
max == min) up front and return the midpoint of the target range,(from + upto) / 2, for all non-NA elements instead. This matches the well-established convention used byscales::rescale()for the identical degenerate-input case (verified:scales::rescale(c(5,5,5), to = c(0,100))returnsc(50,50,50)) — using an existing, widely-used package's behavior as the precedent avoids guessing a novel convention for this edge case.Verification
rescale(1:5)still gives0 25 50 75 100.NaN.NAelements are left asNArather than being assigned the midpoint.rescale(c(2,2), from=10, upto=20)→15 15).R CMD build .succeeds.2.8.16→2.8.20(distinct from sibling PRs Add missing @export to pU() #75/Fix getCategories() always returning an empty vector #76/Fix select_rows_and_columns() silently returning a vector for single row/column selections #77, which also branch from the same2.8.16base and already claimed2.8.17/2.8.18/2.8.19).R CMD check's dependency-availability step cannot complete in this sandbox due to a pre-existing, unrelated environment limitation (ReadWriter's dependencyqsfails to compile against the availablestringfishversion here) — same issue already documented on PR Fix broken \link{} in movingAve2()/imovingSEM() @title tags #71/Add missing @export to pU() #75/Fix getCategories() always returning an empty vector #76/Fix select_rows_and_columns() silently returning a vector for single row/column selections #77, confirmed unrelated to this change.Generated by Claude Code