Fix correctness bugs in the gender-inference path (0.9.0) - #29
Conversation
The lookup-table cache was keyed on __state and __year, which were declared and read but never assigned. Only __dataset was written, so a national query issued after a state query passed the invalidation check and silently reused the state-filtered table. Replaces the three ad-hoc attributes with a single (dataset, state, year) key, and collapses the four filter branches into a filter-then-group-by-name form that also guarantees a unique merge key. Missing names reached the model as the literal string "nan": str(None).lower() encodes to real character indices, so a row with no name came back with a confident gender. Names are now normalized to a nullable string column and rows without a usable name stay NaN throughout. Names with no a-z characters encoded to nothing, kept the 0.5 neutral default, and then fell to the male side of the > 0.5 test — so every Devanagari name under an English dataset was labelled male. They now return None/NaN. Re-running on naampy's own output raised twice over: pre-existing columns became _x/_y merge suffixes and the prop_female lookup raised KeyError, and .at[] was handed an Index despite being a scalar-only accessor (it survived only via an internal KeyError fallback, and raised once the column existed). Stale naampy columns are now dropped before the merge and .loc does the assignment. The input frame is also no longer mutated with __first_name. pred_gender/pred_prob are now always present rather than appearing only when some name missed the lookup, so callers no longer need row.get(). The CLI validated --state via an argparse choices= built from list_states(), which downloaded and parsed the full dataset just to construct the parser — even for --help — and checked against v2_1k regardless of --dataset. download_file streamed straight to the target, so an interrupted transfer left a truncated file that load_naampy_data reported as a valid cache on every later run. It now writes to a temp file and renames on success. Adds tests/test_regressions.py: 17 offline tests over a fabricated fixture, 10 of which fail against the previous implementation. Dataverse-downloading tests move behind NAAMPY_NETWORK_TESTS=1 so the default suite is hermetic. Also drops the dead find_ngrams helper, the unused tensorflow pin in the Streamlit demo requirements and the TensorFlow mocks in the Sphinx config (nothing has imported TF since the PyTorch migration in 0.8.0), and corrects the documented state counts (31, not 30) and the maharastra state key, which the docs spelled maharashtra — a value the data never contained. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e04646e38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if state is not None: | ||
| adf = adf[adf.state == state] |
There was a problem hiding this comment.
Preserve the empty-string all-states sentinel
When the default all option is selected in streamlit/streamlit_app.py, that caller deliberately passes state=""; changing this predicate from truthiness to is not None treats the sentinel as an actual state and filters the lookup table to zero rows. Consequently, the default Streamlit flow loses all electoral-roll counts and falls back to ML for every readable name. Treat the empty string like None, or update the Streamlit caller in the same change.
Useful? React with 👍 / 👎.
| [project] | ||
| name = "naampy" | ||
| version = "0.8.0" | ||
| version = "0.9.0" |
There was a problem hiding this comment.
Regenerate uv.lock after the version bump
The tracked uv.lock still records the editable project as naampy v0.8.0 even though this manifest now declares 0.9.0. Confirmed with uv tree --frozen—whose help describes the option as displaying requirements without locking—which reports naampy v0.8.0; frozen consumers therefore see stale resolution metadata, while non-frozen workflows must rewrite the committed lock. Regenerate and commit uv.lock with this release bump.
Useful? React with 👍 / 👎.
Turning filterwarnings up to a blanket "error" broke the 3.14 job: pandas 2.2
guards against chained assignment by testing sys.getrefcount(self) <= 3 inside
DataFrame.__setitem__, and Python 3.14's refcounting changes make that
heuristic fire on every ordinary df[col] = value. The warning says nothing
about this code and cannot be avoided from here while the pandas pin holds at
<2.3.0, so it is ignored by message with a note to drop the entry once the pin
moves.
Also swaps Series.replace("", pd.NA) for Series.mask(== ""), which avoids
replace's downcasting deprecation and type-checks cleanly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the previous commit's filterwarnings ignore with the actual fix. The pin was pandas>=1.5.0,<2.3.0, a ceiling introduced incidentally by the uv/furo packaging commit rather than for any recorded compatibility reason. It held CI's Python 3.14 job on pandas 2.2.3 — a September 2024 release with no 3.14 wheels, which uv was compiling from source. That version guards chained assignment by testing sys.getrefcount(self) <= 3 inside DataFrame.__setitem__, a heuristic 3.14's refcounting changes break, so it fired on every ordinary df[col] = value. Raising the pin to >=2.0.0 resolves pandas 3.0.5, where copy-on-write is the default and the warning machinery is gone entirely. filterwarnings goes back to a plain ["error"] with nothing suppressed. Verified on pandas 3.0.5 across both CI interpreters: 30 passed / 27 skipped offline on 3.11 and 3.14, 57 passed against the live Dataverse datasets, pyright 0 errors, ruff/mypy/pydoclint clean, sphinx 0 warnings, wheel builds. Adds the Python 3.14 classifier, which CI already tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audit of
naampy/in_rolls_fn.py,nnets.py, andutils.pyturned up several genuine correctness bugs. Each fix has a regression test that fails againstmaster— 10 of the 17 new tests intests/test_regressions.pydo.Bugs fixed
Stale cache returned wrong numbers.
__stateand__yearwere declared and read in the cache-invalidation check but never assigned — only__datasetwas written. So:Against real data,
priyahasn_female=23652in Kerala and41097nationally. The second call returned Kerala's number. Replaced with a single(dataset, state, year)key, and the four filter branches collapse to filter-then-group-by-name, which also guarantees a unique merge key.Missing names were scored as the string
"nan".str(None).lower()→"nan"→encode_name→[14, 1, 14], a real model input. Rows with no name came back with a confident-looking gender. Names are now a nullable string column; rows with no usable name stayNaNthroughout.Non-Latin names were labelled "male". No
a-zcharacters → empty encoding → the0.5neutral default →probs > 0.5is False →"male"at 0.5 confidence. Every Devanagari, Gujarati, and Kannada name under an English dataset. NowNone/NaN.Re-running on naampy's own output crashed, two ways. Pre-existing columns became
_x/_ymerge suffixes and theprop_femalelookup raisedKeyError; and.at[]was handed anIndexdespite being a scalar-only accessor — verified on pandas 2.2.3 that it survived only via an internalKeyErrorfallback and raisesInvalidIndexErroronce the column exists. Stale columns are dropped before the merge;.locdoes the assignment.The caller's DataFrame was mutated with a
__first_namecolumn thatdel rdf[...]only removed from the merged copy.Truncated downloads poisoned the cache permanently.
download_filestreamed straight to the target, so an interrupted transfer left a short file thatload_naampy_datathen reported asUsing cachedon every later run. Now writes to a temp file and renames on success, withraise_for_status()and aContent-Lengthcheck.Behavior changes
pred_gender/pred_probare now always present (exceptv2_native), holdingNone/NaNwhere unused. They previously appeared only when some name missed the lookup, so callers had to probe withrow.get(...).--stateis validated after parsing, against the dataset named by--dataset. It was an argparsechoices=built fromlist_states(), so--helpdownloaded and parsed the full 60MB v2_1k dataset, and states were checked against v2_1k regardless of--dataset.1rather than-1(which the shell reports as 255).Cleanup
find_ngramshelper (zero call sites, left over from the pre-LSTM n-gram model).tensorflowpin fromstreamlit/requirements.txtand the TF mocks from the Sphinx config. Nothing has imported TensorFlow since the PyTorch migration in 0.8.0 —streamlit_app.pyimports onlybase64,pandas,streamlit,naampy, anduv.lockhas no TF entries. Dependabot'suvecosystem entry globs Python manifests repo-wide, which is how it kept opening PRs against that dead pin (Update tensorflow requirement from ==2.18.* to ==2.21.* #22).maharastrastate key, which the docs, user guide, and example notebook all spelledmaharashtra, a value the data never contained.Tests
tests/test_regressions.pyis offline: it patchesload_naampy_datato a small fabricated fixture with known counts, so it asserts on exact numbers rather than on whatever the live Dataverse files happen to hold. The pre-existing suite was network-bound with nearly every assertion wrapped inif pd.notna(...)guards and several bareexcept: passblocks, so none of these bugs would have failed a test. Those tests are now behindNAAMPY_NETWORK_TESTS=1and the swallowed assertions are real.Verification
make lint(ruff, mypy, pydoclint) — cleanuv run pyright— 0 errorsruff format --check— cleanpytest— 30 passed, 27 skipped (offline)NAAMPY_NETWORK_TESTS=1 pytest— 57 passed against the real datasetssphinx-build— succeeds with zero warnings (was 2)uv build— wheel and sdist build at 0.9.0Version bumped to 0.9.0: under 0.x, a minor bump is the signal that existing callers see different output. No tag is pushed — publishing to PyPI is a separate, deliberate step.
🤖 Generated with Claude Code