fix: CRMCleaner silently corrupted complex amounts into wrong finite floats (#129) - #135
Conversation
A single np.complex128 cell (Excel/openpyxl formula round-trip) survived the transform-time object-cast retry and reached the currency parser's string path, where str(3+4j) == "(3+4j)" matched the parenthesised- negative rule and produced a plausible but wrong -34.0 (PhilanthroPy-Project#129). Two layers: - element-wise isinstance(v, complex) guard on object columns masks the cells to NaN with a warning before any string cleanup; - had_value is now computed before masking so an all-unparseable column still raises per the documented contract. Verified: repro from PhilanthroPy-Project#129 now yields NaN + UserWarning; full suite 1843 passed / 23 skipped. Signed-off-by: slegarraga <slegarraga@users.noreply.github.com>
|
Good root-cause fix, well tested (the whole-column-still-raises case especially). One gap before merge: AGENTS.md requires a CHANGELOG.md entry under |
Per AGENTS.md, every PR touching philanthropy/ needs an entry under [Unreleased]. Describes the two-layer guard from this branch: element-wise complex masking to NaN with a warning, and preserved raise-on-nothing-parses contract.
|
Done in 384f2ba: added the |
Signed-off-by: slegarraga <slegarraga@users.noreply.github.com>
|
Heads up: main moved (congrats on landing #140!) and this branch picked up a stale-conflict flag, so I merged current |
2 participants
Closes #129
What happened (reproduced on current main)
CRMCleaner.transformturned3+4jinto -34.0 — sign flipped, imaginary part dropped, digits concatenated — via three guards missing in sequence (full root-cause chain in #129).The fix (two layers)
_coerce_currency_to_float: on object columns, cells that are actuallycomplexare masked to NaN with aUserWarningbefore the string cleanup can mangle them. NaN + warning follows the documented "values that still don't parse become NaN" contract.had_valueis now captured before masking, so a column where nothing parses (all-complex included) still raisescould not parse, exactly as documented.Verification (executed)
NaN+ oneUserWarning mentioning "complex"instead of-34.0.Notes for review