Repository navigation
Prep for 4.8.0 release - #234
Merged
Merged
Conversation
Adds a project-level CLAUDE.md pointing agents at the shared R package development conventions. The file is local-only, so ignore it in both git and R CMD build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sampling weights supplied to `s.weights` are now incorporated into the
matching weights for the stratification methods ("exact", "cem" with
`k2k = FALSE`, "full", "quick", and "subclass"), where previously they
only affected propensity score estimation. `get_weights_from_subclass()`
gains an `s.weights` argument and computes stratum masses from weighted
rather than raw counts; constant sampling weights reproduce the previous
results exactly.
Other changes:
* `solver = "symphony"` is no longer allowed with `method = "cardinality"`;
it returned a different solution on every call and did not respond to
`set.seed()`. Rsymphony is dropped from Suggests.
* `method = "subclass"` no longer segfaults when `subclass` is given as a
single quantile such as 0.5. The input check now distinguishes a count
from a quantile vector explicitly, and rejects values that would produce
a single stratum containing every unit.
* `matchit2optimal()` errors informatively when optmatch returns no
matches, instead of carrying an empty result into `subclass2mmC()` and
failing with "negative length vectors are not allowed".
* Fixed a warning in `matchit2optimal()` that printed a literal `tc[1L]`.
* Corrected the spelling of "allotted" in cardinality matching messages.
Tests: new behavior-recording tests for `method = "exact"`, "subclass",
"optimal", "quick", "cardinality", and NULL, and expanded coverage for
"nearest". Snapshots pin `match.matrix`, `weights`, and `subclass` so that
results are held fixed across the upcoming refactor. Test helpers ported
from WeightIt (`expect_no_unexpected_warning()`, `expect_not_equal()`,
`expect_balance_improved()`, `inject_missingness()`) plus
`expect_matchit_condition()`, which collapses cli's line wrapping so long
messages can be asserted in full.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`add_s.weights()` never recomputed the matching weights for the stratification methods: it guarded on `m$method`, but `matchit` objects record the method at `m$info$method` and have no `method` element, so the condition was never true. The guard now reads `info`, and the recomputed weights go through the same normalization `matchit()` applies, gated on the `normalize` value from the original call. `normalize` is recorded in `info` for that purpose rather than recovered from `m$call`, which would fail whenever it was passed as a variable. * `method = "subclass"` now rejects `subclass = Inf` instead of failing in `seq()` with "'length.out' must be a non-negative number". * Removed the remaining SYMPHONY references from the matching methods vignette and from the pkgdown and rhub workflows. * Added `skip_on_cran()` to the snapshot tests, now that `tests/` is included in the build. The skip sits inside `expect_matchit_snapshot()` rather than at the top of each test so the structural checks preceding it still run on CRAN; the snapshots themselves pin values that depend on the installed optmatch, quickmatch, or highs and are not portable. * Added `waldo` to Suggests, used by the test helpers. Tests: new test-add_s.weights.R covering all five stratification methods, the `normalize` interaction, and the methods that must be left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Assertions on arg-generated conditions were split between `.w()`, which made a pattern whitespace-insensitive, and an interim `expect_matchit_condition()`. Both are replaced by helpers named after `arg::err()`, `arg::wrn()`, and `arg::msg()`, and both are removed. The new helpers: * collapse cli's hard line wrapping before matching, so an expected message can be asserted in full rather than by a short prefix chosen to avoid a wrap point; * match literally by default, since arg messages are full of backticks, quotes, brackets, and question marks that a regex would misread; * match on the rlang subclass, so a base-R failure that happens to mention the same words cannot pass for a proper input check; * accept a vector of patterns, so a call that warns more than once is asserted in a single expectation instead of nested `expect_warning()`s; * catch warnings and messages with `withCallingHandlers()` so the expression runs to completion, which removes the duplicate `suppressWarnings()` re-runs several tests needed in order to reach the fitted object afterward; * report an unexpected error as such rather than as a missing warning. Every condition assertion in the test suite is converted. `method = "exact"` gains a test for the pluralized form of the ignored-argument warning, which the split assertions had missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces the hand-written expect_err()/expect_wrn()/expect_msg() from the
previous commit with cobalt's versions, which solve the same problem more
thoroughly:
* matching is case-insensitive. cli capitalizes the first letter of every
message, so the lowercase source text in `arg::err("covariates must
be ...")` would not otherwise match the rendered "Covariates must
be ...". The hand-written versions were case-sensitive and forced the
expected text to be written the way cli renders it.
* output is routed through `capture.output()`, so a `print()` under test
cannot bury the reporter's own output.
* `squish()` is available on its own for collapsing cli's wrapping.
`expect_wrn()` keeps only the first warning, so the five tests that pin two
warnings from a single call go back to testthat's nested
`expect_warning()`, which re-signals what it does not match. Everything
else uses the cobalt helpers.
`subclass` input validation also now rejects `NaN` and `NA_real_`, which
previously reached base R's "missing value where TRUE/FALSE needed".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No behavior changes: all 381 tests pass with no snapshot churn, and R CMD
check is clean. See _dev/cpp-cleanup-notes.md for the measurements and for
the things deliberately left alone.
Bugs fixed:
* `Function("order")` resolved from the global environment, so a user
function named `order` (or a package masking it) would be called instead
of `base::order()`. Bound to the base environment at all seven call
sites. This affected nearest neighbor matching, optimal matching, and
`summary()` with `pair.dist = TRUE`, and is the only fix here a user
could observe, so it is the one with a NEWS entry.
* `std::isfinite()` on an integer NA in `subclass2mmC()` never fired, since
NA_INTEGER is INT_MIN, which is finite as a double. Masked by the check
on the following line, so no wrong results.
* `preprocess_matchC()` reserved n1 * n0 ints up front (~100 MB at
5000 x 5000) in the function whose purpose is to avoid the dense product.
* `has_n_unique_()` read `x[0]` and wrote `seen[0]` without checking that
`x` is non-empty or that `n >= 1`. Unreachable from current R code.
* `eucdistC_N1xN0()` used `double` loop variables as matrix indices.
* `weights_matrixC()` compared a raw `focal` against a `treat` recoded to
0..g-1; the identity for the 0/1 coding `matchit()` produces, so latent.
* `subclass_scootC()` now breaks out of its search loop instead of writing
an out-of-range subclass if its invariant is ever broken.
Performance. Nearest neighbor matching on the Mahalanobis distance is
40-60% faster; every other method is unchanged (+/-3%):
* `euc_dist_sq()` took two NumericVectors, so `euc_dist_sq(mah_covs.row(i),
mah_covs.row(j))` allocated two R vectors for every candidate control
considered. It now takes the matrix and two row indices. This is the
whole of the speedup.
* Other R allocations removed from per-candidate and per-match paths: the
row copies in `antiexact_okay()`, `na_omit()` in `mm_okay()`,
`mm(row, sum(!is_na(mm(row, _))))` in all six matching files, and the
per-unit `max(as<IntegerVector>(n_eligible[g_c]))`.
* The second `order()` call in four files computed the inverse of a
permutation, which is now done directly in O(n).
* `get_splitsC()` grew its result with `NumericVector::push_back()`, which
reallocates and copies on every call; it now uses a std::vector.
* `subclass2mmC()` is O(n + n1) instead of O(n * n1), via a subclass-to-row
lookup built once. It keeps the first matching row, as the old `break`
did.
`base::order()` was kept everywhere it is used. Four replacements were
written and verified byte-identical to it on adversarial and fuzzed input;
all of them lose by 3-8x, because `order()` on a double vector is a radix
sort while `R_orderVector1()` and the C++ comparison sorts are not. The
call sites now say so.
Cleanups:
* internal.cpp includes internal.h, so declaration/definition drift is a
compile error rather than a link error; this immediately caught a
duplicated default argument. Defaults now live only in the header, and
<numeric> and <vector>, previously arriving only through Rcpp.h, are
included explicitly.
* Scalars are passed by value rather than as `const int&` and friends, in
internal.{h,cpp} and the small standalone files; the six exported
nn_matchC_* signatures are left alone to keep the diff there small.
* `using namespace Rcpp;` is out of internal.h, which every translation
unit includes; its declarations are Rcpp::-qualified.
* The duplicated ~50-line tail of `find_control_vec()` and
`find_control_mat()` is one `take_closest()` helper. It keeps the
`partial_sort()`/`sort()` split as it was: those order ties differently,
and collapsing them would change which control lands in which column of
match.matrix for tied distances.
* Removed the C++ interface in inst/include, generated by sixteen
misapplied `interfaces(cpp)` attributes. It exported no functions, its
validate table was empty, no package declares LinkingTo: MatchIt, and the
`methods::setLoadAction()` call it put in RcppExports.R referenced a
package not in Imports.
* Removed src/Makevars and src/Makevars.win, which passed OpenMP, BLAS,
LAPACK, and FLIBS flags with no omp pragma or BLAS call anywhere in src/.
* Removed dead code, the inert `plugins(cpp11)` attributes, and narrowing
conversions; added missing trailing newlines.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
No description provided.