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

`Summary`, `vcov`, and `confint` methods for `DirectAdjusted` objects #66

Closed jwasserman2 closed 2 years ago

jwasserman2 commented 2 years ago

I know we have a lot to discuss about how to make these play with different standard error calculations users may want, but for now I set these up very basically to use logic based on whether the DirectAdjusted model has a SandwichLayer offset or not. DirectAdjusted objects with SandwichLayer offsets will use vcovDA to calculate standard errors, and those without will use default OLS standard errors.

Since confint uses stats::vcov's S3 dispatch to calculate SE's, I changed vcov.DirectAdjusted and confint.DirectAdjusted to S3 methods to reduce the extra code I had to write. We'll want to discuss our tolerance for exceptions to S4 methods at some point, since I also had to add estfun.lmrob and bread.lmrob as S3 methods.

benthestatistician commented 2 years ago

I agree about tapping into the S3 mechanisms where possible, to limit the need to write new code among other reasons. (For example, Hadley W argues, convincingly I think, that whereever possible user-facing methods should be S3 rather than S4.). However, if there are competing reasons to use S4, I'm aware that @josherrickson knows some tricks for getting S3-type dispatch on S4 objects. (Josh E do expand, here or offline, if you see this as potentially relevant.)

josherrickson commented 2 years ago

I'm out of town (with only my phone) until after the Labor Day weekend. I remember making some decisions about S3 vs S4 for those methods but I'd have to review the differences. One major benefit of S4 is being able to dispatch on two arguments, but that may not be relevant here.

I'll review when I return but if this is blocking feel free to push as long as it passes checks.

josherrickson commented 2 years ago

I see the following warnings:

❯ checking dependencies in R code ... WARNING
  ':::' call which should be '::': ‘stats:::confint.lm’
    See the note in ?`:::` about the use of this operator.
  Unexported object imported by a ':::' call: ‘stats:::vcov.lm’
    See the note in ?`:::` about the use of this operator.
    Including base/recommended package(s):
    ‘stats’

❯ checking S3 generic/method consistency ... NOTE
  Found the following apparent S3 methods exported but not registered:
    summary.Design
  See section ‘Registering S3 methods’ in the ‘Writing R Extensions’
  manual.

❯ checking R code for possible problems ... NOTE
  summary,DirectAdjusted: no visible global function definition for ‘pt’
  Undefined global functions or variables:
    pt
  Consider adding
    importFrom("stats", "pt")
  to your NAMESPACE file.

❯ checking Rd \usage sections ... NOTE
  S3 methods shown with full name in documentation object 'summary.Design':
    ‘summary.Design’

  The \usage entries for S3 methods should use the \method markup and not
  their full name.
  See chapter ‘Writing R documentation files’ in the ‘Writing R
  Extensions’ manual.
josherrickson commented 2 years ago

I looked into S3/S4 a bit; I see no reason for now not to proceed with @jwasserman2's plan. As soon as it passes checks, go ahead and merge it in.