fix(epc): one property certified twice is not two properties - #65
Merged
Conversation
Reproduced live against the real EPC API:
EPCClient().search_by_postcode('NG11 9HD', address='27 Havenwood Rise')
-> EPCAmbiguousMatchError: 2 certificates share the address text
'27 Havenwood Rise'; cannot select one
Both candidates carry UPRN 100031555077. One house, certified twice, whose
address text differs only by a comma. Properties are re-certified on every sale
and let, so this is the normal case rather than an edge one, and every affected
property was unreachable by address -- including through enrich_comps_with_epc,
where it silently left comps un-enriched and depressed epc_match_rate.
Measured on real data across six postcodes, 147 distinct addresses:
NG11 9HD 30 addresses 12 resolvable -> 30
M1 1AE 28 addresses 11 resolvable -> 28
DE12 6LL 13 addresses 10 resolvable -> 13
B5 4BX 76 addresses 76 resolvable -> 76 (no duplicates; unchanged)
TOTAL 147 addresses 109 (74%) -> 147 (100%)
B5 4BX is the control: where no property holds more than one certificate,
nothing changes. This is not a blanket relaxation.
Both entry points were affected. The caller-supplied-UPRN branch refused
identically ("N certificates share UPRN X"), so fixing only the address path
would have left epc_lookup(uprn=...) broken for the same properties.
The rule is deliberately narrower than "same UPRN wins". All three must hold:
* every candidate carries a non-empty UPRN and they all agree. UPRN is
optional upstream and often absent, so two blanks are not agreement.
* the candidates agree on canonical address text. A shared UPRN with
DIFFERENT addresses is contradictory upstream data, not one property;
choosing there would be precisely the failure this module exists to
prevent -- attaching another property's floor area and every
price-per-sqft derived from it.
* the registration dates order strictly. A tie has no "most recent", and
resolving one by upstream row order is a defect the module docstring
already catalogues. Dates are parsed with a canonical ISO round trip,
not compared as strings -- ppd_source.validate_date_range carries the
scar for that.
Consequently every existing guard passes UNMODIFIED, including
test_duplicate_exact_addresses_remain_ambiguous (uprn=None),
test_duplicate_uprn_is_ambiguous and test_ambiguous_uprn_does_not_pick_arbitrarily
(both pair different addresses under one UPRN). That was the design check: this
module has been repaired four times, each round finding a new way for partial
evidence to look sufficient, and a fix that needed those tests relaxed would be
the fifth. Rule 4 adds no property evidence at all -- it applies only once
identity is established to rules 1-3's standard, then chooses within that one
property's own certificate history.
Reported as method "uprn_latest_certificate" at confidence 100: identity is
certain, and the method name discloses that the newest of several certificates
was taken.
Also wired up the `ambiguous` counter in enrichment.py, which was incremented
and never read -- so how often selection refused, and therefore what any change
to it is worth, could not be measured from anything that function left behind.
Now logged with a per-method breakdown.
Expect epc_match_rate to rise and median_price_per_sqft to move, since
newly-matched comps bring floor areas into the median.
./scripts/validate.sh: 2140 passed, 28 skipped.
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.
Reproduced live
Both candidates carry UPRN
100031555077. One house, certified twice, whoseaddress text differs only by a comma. Properties are re-certified on every sale
and let, so this is the normal case — and every affected property was
unreachable by address, including through
enrich_comps_with_epc, where itsilently left comps un-enriched.
Measured on real data, six postcodes
B5 4BX is the control — where no property holds more than one certificate,
nothing changes. This is not a blanket relaxation.
Both entry points were broken
The caller-supplied-UPRN branch refused identically (
N certificates share UPRN X), so fixing only the address path would have leftepc_lookup(uprn=...)broken for the same properties.
The rule is deliberately narrow
All three must hold:
optional upstream and often absent, so two blanks are not agreement;
different addresses is contradictory upstream data, not one property, and
choosing there would be exactly the failure this module exists to prevent;
resolving one by upstream row order is a defect the module already catalogues.
Dates get a canonical ISO round-trip, not a string compare
(
ppd_source.validate_date_rangecarries the scar for that).The design check
Every existing guard passes unmodified — including
test_duplicate_exact_addresses_remain_ambiguous(uprn=None),test_duplicate_uprn_is_ambiguousandtest_ambiguous_uprn_does_not_pick_arbitrarily(both pair different addressesunder one UPRN).
That mattered. This module has been repaired four times, each round finding a
new way for partial evidence to look sufficient, and a fix needing those tests
relaxed would have been the fifth. Rule 4 adds no property evidence at all: it
applies only once identity is established to rules 1–3's standard, then chooses
within that one property's own certificate history.
Reported as
uprn_latest_certificateat confidence 100 — identity is certain,and the method name discloses that the newest of several certificates was taken.
Also
The
ambiguouscounter inenrichment.pywas incremented and never read, sohow often selection refused — and therefore what any change to it is worth —
couldn't be measured from anything that function left behind. Now logged with a
per-method breakdown.
Expect
epc_match_rateto rise andmedian_price_per_sqftto move, sincenewly-matched comps bring floor areas into the median.
./scripts/validate.sh→ 2140 passed, 28 skipped. Live A/B run against thereal EPC API on the deployed Machine, with production code untouched (patched
module loaded from
/tmp).