Closed jwasserman2 closed 9 months ago
Thanks for nicely detailed PR, @jwasserman2. There are errors here and there in the document this PR would update, and I'm unclear on whether/where the update attempts to fix these errors. An example is the display following eq:10 in the "Realization of Goal" subse ction, p.6:
That's how it read before the changes you propose. I've shown it as it was at the time of your updates; just now I've pushed a fix to the main branch, changing that passage to: This retains the scalings for $M{11}$, $M{21}$, $M_{22}$ that were written into the original spec, rather than the [non-] scalings of the proposed update. The proposed update has and I'm unclear whether I should take the $n^{-1}$ scaling as written or adjust it as I do in the fix described above. It would be OK for the update to start with yet another scaling, in the interests of maintaining correctness and coherence of the presentation.
(Along with the fix described above that I'm pushing to the main branch, I'm also pushing a few smaller changes to both this branch and main, in both cases aimed at maintaining internal coherence of the spec.)
A connection that the spec update seems to break: In the original, eqn 9 as below follows from eqn 8: (I added the equation number for the second display only just now, with [main 5f172b8] / [spec_updates_jw fc3d498].) In the proposed revision, 9 no longer follows from 8:
The entailment of (9) by (8) (see previous comment) would be restored by updating the definition of $A_{11}(\beta)$ just above these displays to read $\sum\nabla\phi(z_i, \beta)$ not $n_C^{-1}\sum\nabla\phi(zi, \beta)$, and similarly by updating the $\hat{A}{21}(\beta)$ definition from $n^{-1}\sum\nabla\psi(z_i,\tau,\alpha,\beta)$ to $\sum\nabla\psi(z_i,\tau,\alpha,\beta)$. That would align the spec with the code base as it now stands.
This highlights the unusual interpretations of $A$ being used in the current codebase. Whatever the advantages of this approach, it's confusing now and will be highly confusing in the future. As long as .get_a11_inverse()
and .get_a21()
are returning an unscaled covadj bread matrix and an unscaled sum of gradients, let's call those functions something like .get_a11_inverse_unscaled()
and .get_a21_unscaled()
. This is a change that can be made in the main branch now, even as we consider whether to update the spec to reflect the code (as proposed in this PR) versus updating the code to reflect the spec.
(Note for those deliberations: I suspect that the sandwich package is using scaled versions of these matrices because that's expected to make computer arithmetic more reliable as sample sizes get large.)
Per offline discussion, closing this PR.
This branch updates the spec document to reflect the way I've coded
estfun.DirectAdjusted()
,.get_a11()
, and.get_a21()
in DirectAdjusted.R and SandwichLayerVariance.R, respectively. I've posted screenshots below that compare the changes between the original and updated documents in the section that derives these expressions. I've also updated expressions later in the document that reference the $(n/n_{\mathcal{C}})^{1/2}$ scaling constant, but I haven't added screenshots here. This includes changes to the sections of the document discussing design-based variance estimation, so @xinhew0708, in addition to taking a look over my changes here, I'm also tagging you to take a look and make sure this doesn't change the way you've written your design-based code.Original Spec Doc (pg. 5)
Updated Spec Doc (pg. 5)
Original Spec Doc (pg. 6)
Updated Spec Doc (pg. 6)
Original Spec Doc (pg. 7)
Updated Spec Doc (pg. 7)