Fix calc_prb: return the PRB slope instead of the intercept - #392
Open
nikhilsi wants to merge 1 commit into
Open
Fix calc_prb: return the PRB slope instead of the intercept#392nikhilsi wants to merge 1 commit into
nikhilsi wants to merge 1 commit into
Conversation
sm.add_constant prepends the constant column, so params[0] is the intercept and params[1] is the PRB coefficient. Read index 1 in the degenerate-fit guard, the returned statistic, and the confidence interval. Adds a regression test that fits the same transformed regression by hand and checks a biased set and a flat control.
Contributor
|
Thank you for your contribution. I affirm that this contributor has signed the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
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.
What was wrong
In
openavmkit/utilities/stats.py,calc_prbbuilds the PRB regression withsm.add_constant(right, has_constant='add')(line 579 at 7952236), whichprepends the constant column. So
model.params[0]is the intercept andmodel.params[1]is the PRB coefficient. The function reads index 0throughout:
model.params[0]prb = float(model.params[0])Every PRB value the library reports today is the regression intercept, which
carries no vertical-equity signal. The sign and magnitude are unrelated to
price-related bias.
The change
Index 1 instead of index 0 in all three places (guard, coefficient,
confidence interval). Three lines, no behavior change elsewhere.
One related note for reviewers: the value proxy on line 578 omits the 0.5
scaling from the IAAO formulation. Under log2 that only shifts the intercept,
not the slope, so it does not affect this fix and is left untouched here.
What changes downstream
All reported PRB numbers change, including in
ratio_study.py(lines125-126),
vertical_equity_study.py(line 111), andmodeling.py(line263). That is the point of the fix: the previous numbers were intercepts.
Anyone tracking PRB across runs will see a step change after upgrading.
Verification
A test is included (
tests/test_stats.py). It builds a synthetic ratio setwith a known value-correlated bias (predictions overshoot high-value parcels
and undershoot low-value ones), fits the same transformed regression by hand
with
sm.OLS, and asserts three things:calc_prbmatches the hand-fitparams[1]to within float tolerance, the sign matches the injected biasdirection, and an unbiased control set (predictions equal to a constant
multiple of ground truth) yields a PRB near zero. At 7952236 the biased case
fails (the intercept comes back instead of the slope); with this change it
passes.
Fixes #370.