benbhansen-stats / propertee

Prognostic Regression Offsets with Propagation of ERrors, for Treatment Effect Estimation (IES R305D210029).
https://benbhansen-stats.github.io/propertee/
Other
2 stars 0 forks source link

tracking for changes in functions that we've copied-and-slightly-modified #164

Closed josherrickson closed 6 months ago

josherrickson commented 7 months ago

This came up in #163 - we have a number of functions that are copies-with-small-tweaks. It'd be nice to be able to at least know if these functions change in base R so we can decide whether we keep consistent or do something else.

There may be more, but the functions I'm thinking of are:

I've got three potential solutions:

  1. Use some sort of online service to monitor changes on the r-source repo. Con: Can we make it targeted enough that changes in GitHub's UI and/or changes elsewhere in the script don't trigger warnings? Pro: this is external to the package checking, so a minor change (e.g. a spacing change) won't require an instantaneous fix to pass tests.
  2. Prior to testthat, the way testing in R worked was you wrote a script, saved its output, then when calling R CMD check, it would re-run the script and compare the results against the saved output. Pro: Simple. Con: Is this even supported anymore? A quick perusal of Writing R Extensions manual doesn't even seem to mention it.
  3. Do this in testthat: Save the results of a call to, e.g. expand.model.frame, then call expect_identical(saved_results, capture.output(expand.model.frame)). I think this is probably the best option.
josherrickson commented 7 months ago

To be clear, I'm not talking about situations where we pulled out some code from function A to use in completely different function B. I'm talking about situations where, for example, there's an existing function base::function, and we copy the code to create propertee::.function, with the idea that calls to both should be identical, except for a small tweak we made. If base::function changes it's code, we could end up in a weird drifting state where we're producing results that are not consistent with base functionality in a way that would be hard to discover without these preemptive tests.

benthestatistician commented 7 months ago

Good thinking. Option 3 makes most sense to me.

Message ID: @.***>

jwasserman2 commented 7 months ago

I like option 3 as well

josherrickson commented 6 months ago

I implemented this. I only did it for expand.model.frame and confint.lm; the robustbase functions seemed to be more modified/copied for specific purposes and less generic. @adamSales please let me know if you think I should include them in these checks.