dataprep() applies predictors.op to the treated unit but not the controls - #2
Open
soodoku wants to merge 1 commit into
Open
dataprep() applies predictors.op to the treated unit but not the controls#2soodoku wants to merge 1 commit into
soodoku wants to merge 1 commit into
Conversation
dataprep() aggregated the treated unit with the caller's predictors.op (R/dataprep.R:216) but the controls with a hard-coded mean (R/dataprep.R:257), so any operator other than "mean" produced a predictor-balance matrix whose treated and control rows were computed on different rules. With the bundled basque data, predictors.op = "median" and time.predictors.prior = 1964:1969, the treated unit's X1 matches the hand median exactly while every control's X0 matches the hand mean exactly, off by up to 2.06 from the median that was requested. Adds tests/testthat/test-dataprep-predictors-op.R, which fails without this change and passes with it. Full suite unchanged at FAIL 0 | SKIP 3 | PASS 187.
soodoku
added a commit
to finite-sample/synth-predictors-op-bug
that referenced
this pull request
Aug 2, 2026
An earlier version of this README stated Synth has no public issue tracker. That was wrong -- j-hai/Synth exists and is active. I had trusted the URL field in the DESCRIPTION, which lists only the author's homepage, rather than checking the maintainer's GitHub account directly. The fix is now submitted upstream as j-hai/Synth#2. 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
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.
dataprep()aggregates predictors overtime.predictors.priorseparately for the treated unit and the controls. The treated unit uses the operator the caller supplied:The controls use a hard-coded
mean:So for any
predictors.opother than"mean",X1andX0are built under different aggregation rules and the predictor-balance matrix thatsynth()optimises over is internally inconsistent.Reproduction
Bundled
basquedata,predictors = "invest",predictors.op = "median",time.predictors.prior = 1964:1969:On
main(1.2-0):Asking for the median moves only the treated unit's row; every control row is byte-identical to what the
"mean"call produces.Change
One line, mirroring what line 216 already does:
With it,
max |X0 - hand median|becomes0.000000.Two things suggested this is an oversight rather than a deliberate asymmetry, though you would know better than I do:
predictors.op" (R/dataprep.R:252) — in the same block that hard-codesmean.special.predictorsoperators are applied to both the treated unit and the controls (R/spec.pred.func.R), so the package currently treats the two kinds of predictor differently.?dataprepdescribespredictors.opas the method used to aggregate the predictor variables, with no treated/control distinction.Verification
tests/testthat/test-dataprep-predictors-op.R: FAIL 1 / PASS 1 without this change, PASS 2 with it.cd tests && NOT_CRAN=true Rscript testthat.R: FAIL 0 | SKIP 3 | PASS 187 both before and after.R/dataprep.Ris byte-identical between CRAN 1.1-10 andmainat 1.2-0, so this affects the released version too.What I have not established
Whether the asymmetry is intended — it is possible you want control units summarised by their mean regardless of
predictors.op. If so the right change is a documentation note rather than this patch, and I am happy to be told that.