fix(sec): keep the market-cap fallback through an amendment merge
Found in review. _merge_amendments rebuilds a period from _MERGED_FIELDS + _CARRIED_FIELDS alone, so a column in neither list is absent from the merged row, not just stale — and callers read it with getattr(..., None), which silently yields None. weighted_avg_diluted_shares was never added when the market-cap fallback landed (_SNAPSHOT_COLS in the importer was updated, its counterpart in the derivation was not). The failure needed both of this branch's fixes at once: a multi-class issuer with a partial amendment on its latest period (META with a Part-III-only 10-K/A) would silently lose market cap and FCF yield again. Adds the field, a regression test for that case, and a guard test asserting the merge/carry lists cover every SnapshotRow field, so the next column added fails loudly rather than losing data quietly. Confirmed the guard catches the original bug. Also from review: - Expose pe_caveat in the valuation payload, so a P/E suppressed by split contamination says why instead of looking like missing data (the caveat was set but never read). - no_xbrl_filings now names both causes; the old text advised pinning a CIK override, which is wrong for a genuine new registrant that simply has not filed yet and clears itself. - Document that fiscalYearEnd is the issuer's current calendar, so a fiscal- year-end change degrades old periods (fallback/newest-wins), not current ones. - Parser-level tests for _select_weighted_avg_shares (shortest-span-wins and concept priority), which only had derivation-level coverage. 823 unit tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -801,6 +801,29 @@ data limitation and two operational steps that only run against production.
|
||||
8. ~~**PPL basic-EPS fallback**~~ — **done**, ninth pass.
|
||||
9. ~~**HAL null TTM EPS**~~ — **resolved**, ninth pass (already fixed; 2024 is a filer error).
|
||||
|
||||
## Review finding — two fixes on this branch silently interacted
|
||||
|
||||
Caught in review, not by me. `_merge_amendments` (the per-field amendment fix, first pass)
|
||||
builds the merged period from `_MERGED_FIELDS` + `_CARRIED_FIELDS` alone, so a column in
|
||||
neither list is **absent** from the merged row, not merely stale — and every caller reads it
|
||||
with `getattr(row, name, None)`, which quietly returns `None`.
|
||||
|
||||
`weighted_avg_diluted_shares` (the market-cap fallback, seventh pass) was never added to
|
||||
`_MERGED_FIELDS`. The failure needed both fixes to be present at once: a multi-class issuer
|
||||
*and* a partial amendment on its latest period — META with a Part-III-only 10-K/A — would
|
||||
silently lose market cap and FCF yield again, i.e. the seventh pass's fix undone by the
|
||||
first pass's mechanism. I updated `_SNAPSHOT_COLS` in the importer when adding the column
|
||||
but not `_MERGED_FIELDS` in the derivation.
|
||||
|
||||
Fixed, with a regression test for the specific case. The more useful addition is a guard —
|
||||
`test_merge_lists_cover_every_parser_field` asserts the two lists cover every `SnapshotRow`
|
||||
field, so the *next* column added fails loudly instead of losing data quietly. Verified it
|
||||
would have caught this one.
|
||||
|
||||
Lesson worth keeping: a hand-maintained field list that reconstructs an object is a silent
|
||||
data-loss footgun. `_SNAPSHOT_COLS` (importer) and `_MERGED_FIELDS` (derivation) must both
|
||||
track the parser's `SnapshotRow`, and only one of them is now enforced by a test.
|
||||
|
||||
## Genuinely unfixable from this data
|
||||
|
||||
- **§1 part (b)** — a split post-dating the last filing (KLAC). No snapshot carries
|
||||
|
||||
Reference in New Issue
Block a user