Add tests for reduce_dimensions() - #36
Open
ck37 wants to merge 1 commit into
Open
Conversation
tests/testthat/test-reduce_dimensions.R was a single-line TODO, leaving
the HOPACH dimension-reduction wrapper - the one piece of the pipeline
that decides which covariates every downstream model sees - without any
coverage.
Adds 6 tests / 18 assertions covering the paths that matter to callers:
the pass-through when the data is already narrow enough, constant-column
removal from both frames, reduction when there are more columns than the
cutoff, newX staying aligned with data in name and order, absent newX
columns being filled with zeros in data's order, and a NULL
max_variables disabling reduction.
Two assertions are narrower than they look, and the comments say why:
- max_variables is not an upper bound. HOPACH selects the highest level
of the tree holding at least max_variables clusters, so 20 columns
with max_variables = 5 comes back as 9. The test asserts only that the
result is narrower than the input, because asserting <= max_variables
would assert something the function does not do. The roxygen ("Maximum
we want to allow") and the varimpact() argument name adjust_cutoff
both read as a cap, so this is worth a look separately.
- newX is not exercised at its documented default of NULL, because that
default does not work: reduce_dimensions(data, max_variables = 10)
fails in matrix(0, nrow = nrow(Wvsht), ...) with "non-numeric matrix
extent", since a NULL newX reaches the missing-column fill-in with
nrow(NULL). Left as-is rather than asserting the error, which would
enshrine a bug.
No behavior changes - tests only.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xo3yCjZifUfAfLdHL1c1Hu
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.
Tests only — no behavior changes.
Why
tests/testthat/test-reduce_dimensions.Rwas a single line:# TODO: need to test reduce_dimensions function.That left the HOPACH dimension-reduction wrapper — the piece that decides which covariates every downstream model actually sees — with no coverage at all.
What this adds
6 tests / 18 assertions covering the paths callers depend on:
dataandnewX(they have to stay aligned)newXstaying aligned withdatain name and ordernewXfilled with zeros, ordered to matchdatamax_variables = NULLdisabling reductionTwo things I found while writing these
Both are asserted narrowly on purpose, and the test comments say why. Neither is fixed here — they are behavior questions, not test questions.
1.
max_variablesis not an upper bound. HOPACH selects the highest level of the tree holding at leastmax_variablesclusters, so 20 columns withmax_variables = 5comes back as 9. The test asserts only that the result is narrower than the input; asserting<= max_variableswould be asserting something the function does not do.This matters beyond this function: the roxygen reads "Maximum we want to allow", and
varimpact()exposes it asadjust_cutoff = 10L. Both read as a cap, so a user asking for at most 10 adjustment covariates may get more.2.
newXdoes not work at its own documented default ofNULL. The signature isreduce_dimensions(data, newX = NULL, max_variables, verbose = FALSE), but:With
newX = NULL,WvshtstaysNULL, so every column ofWtshtlands inmissing_colsand the fill-in at R/reduce_dimensions.R:151 callsmatrix(0, nrow = nrow(NULL), ...). Insidevarimpact()this never fires becausenewXis always supplied. I left it out rather than adding anexpect_error, which would enshrine the bug. Happy to fix it in a follow-up if you want the default to work.Verification
Green on
master(34 assertions across the full suite, 0 failures) and green on the branch for #34 (18 assertions, 0 failures).🤖 Generated with Claude Code
https://claude.ai/code/session_01Xo3yCjZifUfAfLdHL1c1Hu
Generated by Claude Code