Skip to content

Fix VEI crash on degenerate market proxy (qcut 'Bin edges must be unique') - #369

Open
ABtheMD wants to merge 1 commit into
larsiusprime:masterfrom
ABtheMD:fix/vei-degenerate-group
Open

Fix VEI crash on degenerate market proxy (qcut 'Bin edges must be unique')#369
ABtheMD wants to merge 1 commit into
larsiusprime:masterfrom
ABtheMD:fix/vei-degenerate-group

Conversation

@ABtheMD

@ABtheMD ABtheMD commented Jul 2, 2026

Copy link
Copy Markdown

Problem

get_vertical_equity_scores (openavmkit/vertical_equity_study.py) raises
ValueError: Bin edges must be unique and aborts the entire modeling run
whenever the VEI "market proxy" is degenerate but the sample has ≥ 20 rows (so
the existing small-sample guard doesn't apply). Two real triggers:

  1. Valuation all-zero / all-NaN. Hit in practice on a tax-exempt model
    group
    , whose assessed values are $0. Then median_ratio == 0, so
    market_proxy = sale*0.5 + valuation/median_ratio = sale*0.5 + 0/0 = NaN for
    every row, and pd.qcut raises on all-NaN bin edges.
  2. Concentrated values. Many identical / capped / round-number amounts
    (common in assessment rolls) put several quantile boundaries on the same
    edge, so pd.qcut(q=N) raises even when there are enough distinct values
    overall.

Either way, one degenerate group takes down a run where every other group
modeled fine.

Fix

Purely defensive — valid inputs are unaffected; degenerate inputs now degrade to
a NaN result (exactly as the function already does for < 20 observations)
instead of raising:

  • Return the NaN result dict when median_ratio is 0/non-finite or the
    market proxy has < 2 distinct finite values.
  • Pass duplicates="drop" to pd.qcut and index the actual top/bottom
    tiers (group_stats.index.min()/max()) rather than assuming labels
    0..percentile_group_count-1; return NaN if fewer than 2 tiers can be formed.

Tests

Adds tests/test_vertical_equity.py:

  • test_all_zero_valuation_returns_nan_not_crash (the tax-exempt repro)
  • test_constant_market_proxy_returns_nan_not_crash
  • test_concentrated_values_do_not_crash
  • test_healthy_data_still_returns_finite_vei (control — happy path unchanged)

All four pass; test_ensemble.py, test_modeling.py, test_horizontal_equity.py
still green (no regression).

Found while running a full all-class county model (Berks County, PA) on v0.6.0.

…rate market proxy

get_vertical_equity_scores crashed with "ValueError: Bin edges must be
unique" whenever the VEI market proxy was degenerate but had >= 20 rows
(so the small-sample guard didn't apply). This aborted the entire
modeling run. Two triggers:

- valuation all-zero/all-NaN (e.g. a tax-exempt model group with $0
  assessed values) -> median_ratio == 0 -> market_proxy = sale*0.5 +
  val/0 = NaN for every row -> pd.qcut raises on all-NaN edges.
- concentrated values (many identical/capped amounts, common in
  assessment rolls) -> repeated quantile edges -> pd.qcut raises even
  with enough distinct values overall.

Fix: (1) return the NaN result dict (as already done for < 20 obs) when
median_ratio is 0/non-finite or the proxy has < 2 distinct finite
values; (2) pass duplicates="drop" to pd.qcut and index the actual
top/bottom tiers (min/max labels) so concentrated data forms as many
tiers as it can instead of crashing, returning NaN if fewer than 2
tiers result. Valid inputs are unaffected.

Adds tests/test_vertical_equity.py covering all three degenerate cases
plus a healthy-data control.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution.
Please sign our CLA at the following link:
Click here to sign the CLA.

No action is required from you in this PR thread. Once you have signed the CLA externally, a maintainer will verify your signature and record it here on your behalf by commenting:


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.

@ABtheMD

ABtheMD commented Jul 2, 2026

Copy link
Copy Markdown
Author

recheck

@ABtheMD

ABtheMD commented Jul 2, 2026

Copy link
Copy Markdown
Author

The PR was generated by Claude, so I just wanted to add my own human understanding here. I was working on VEI metrics for each of the land classes, when the library crashed on exempt.

Because the vast majority of the exempts have an assessment of $0, which ultimately gives us a whole lot of NaNs when we are trying to calculate the market_proxy. I suppose there's an alternative path where we exclude Exempts from VEI calculation, but this seems cleaner and more foolproof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant