Fix NNS.norm unequal-length list detection and variable assignment#37
Merged
OVVO-Financial merged 1 commit intoJul 5, 2026
Merged
Conversation
Two defects in the list branch of NNS.norm:
1. The nonlinear scale-factor step discarded the cbind result
('do.call(cbind, X)' was never assigned), so cor()/NNS.dep() received
the raw list and errored. Equal-length lists therefore only worked
with linear = TRUE. Assign the bound matrix and use it for the scale
factor, so equal-length lists now support the nonlinear path and
match the equivalent matrix call.
2. Unequal-length detection used sum(diff(lengths)) != 0, which misses
patterns whose diffs cancel (e.g. lengths 5, 6, 5) and let them fall
through to cbind recycling. Use length(unique(lengths)) > 1 instead.
Also label the matrix result from list input via names(X) (colnames() on
a list is NULL, which previously stripped the ' Normalized' labels).
Adds tests/testthat/test_Normalization.R covering the matrix path, the
equal-length list path against the matrix result, forced linear scaling
for unequal lengths, and the zero-sum length-diff case.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JMQ9tSZyAMwerv7BdY6E5w
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
2 participants
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
This PR fixes bugs in the
NNS.normfunction related to detecting unequal-length lists and properly assigning variables in the nonlinear normalization path. It also adds comprehensive test coverage for the normalization function.Key Changes
Fixed unequal-length list detection: Changed from
sum(diff(sapply(X, length))) != 0tolength(unique(sapply(X, length))) > 1to correctly identify when list elements have different lengths. The original approach could miss cases where length differences sum to zero (e.g., +1, -1).Fixed variable assignment in nonlinear path: Added explicit assignment of
X_matwhen converting list input to matrix form. Previously, the result ofdo.call(cbind, X)orXwas not being assigned, causing subsequentcor()andNNS.dep()calls to operate on the wrong data.Improved column name handling: Updated the logic for extracting base column names to handle both list and matrix inputs correctly, using
names(X)for lists andcolnames(X)for matrices.Code cleanup: Fixed trailing whitespace and formatting inconsistencies.
Test Coverage
Added comprehensive test suite (
test_Normalization.R) covering:https://claude.ai/code/session_01JMQ9tSZyAMwerv7BdY6E5w