Skip to content

Add Phase 4: tidy output, MOE scaling, geometry for flows, and survey metadata#298

Merged
chekos merged 2 commits into
mainfrom
claude/competent-kilby
Mar 7, 2026
Merged

Add Phase 4: tidy output, MOE scaling, geometry for flows, and survey metadata#298
chekos merged 2 commits into
mainfrom
claude/competent-kilby

Conversation

@chekos

@chekos chekos commented Mar 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • Implement output="tidy" for get_estimates() and get_flows() (melts value columns into long format, following the pattern in get_acs())
  • Implement moe_level scaling (90/95/99) for get_flows() using z-score ratios
  • Implement geometry=True support for get_flows() via attach_geometry(), with GEOID construction from origin FIPS columns
  • Add get_survey_metadata() function in pypums/survey.py that fetches the Census API discovery endpoint (data.json) and returns available datasets, with optional year filtering
  • Export get_survey_metadata from pypums/__init__.py
  • breakdown_labels and time_series remain documented as not yet implemented (forward-compatible stubs)

Test plan

  • 30 new tests covering all added functionality (4 for estimates output, 8 for flows output/moe/geoid, 8 for survey metadata, plus existing tests updated)
  • All 118 tests pass (uv run --extra test pytest tests/ -v)
  • Pre-commit hooks pass (ruff check + format)

🤖 Generated with Claude Code

… metadata

Implement stubbed parameters across estimates and flows modules:
- output="tidy" support for get_estimates() and get_flows()
- moe_level scaling (90/95/99) for get_flows()
- geometry support for get_flows() via attach_geometry()
- GEOID construction for flows from origin FIPS columns

Add get_survey_metadata() in pypums/survey.py that fetches dataset
catalog from the Census API discovery endpoint, with optional year
filtering.

Export get_survey_metadata from pypums/__init__.py.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Mar 7, 2026

Copy link
Copy Markdown

Claude finished @chekos's task in 2m 0s —— View job


Code Review — Phase 4: tidy output, MOE scaling, geometry for flows, and survey metadata

  • Gather context
  • Read CLAUDE.md (not present)
  • Review changed files
  • Post review feedback

Summary

The implementation is clean and well-tested overall. I found a few bugs worth addressing before merge.


Bugs

🔴 Cache key bug — moe_level and output missing from cache key in flows.py

pypums/flows.py:165-167

cache_key = (
    f"flows_{year}_{geography}_{state}_{county}_{msa}_{vars_str}_{breakdown_str}"
)

The cache stores the post-transformed DataFrame (after MOE scaling and tidy/wide reshaping), but neither moe_level nor output are part of the key. This means:

# First call: caches tidy-formatted data with 95% MOE
get_flows("county", state="CA", county="037", moe_level=95, output="tidy", cache_table=True)

# Second call: incorrectly returns the tidy 95%-MOE cache hit!
get_flows("county", state="CA", county="037", moe_level=90, output="wide", cache_table=True)

Fix this →

🔴 Same cache key bug in estimates.py

pypums/estimates.py:145-148

cache_key = (
    f"est_{vintage}_{resolved_product}_{geography}_{state}_{county}"
    f"_{year}_{vars_str}_{breakdown_str}"
)

output is not included. Calling with output="tidy" then output="wide" (or vice versa) with cache_table=True returns stale results.

Fix this →

🟡 geometry=True with geography="metropolitan statistical area" will raise

pypums/flows.py:236-239

if geometry:
    from pypums.spatial import attach_geometry
    df = attach_geometry(df, geography, state=state, year=year)

_GEO_TO_TIGER in spatial.py has no entry for "metropolitan statistical area" (only "cbsa"). Calling get_flows("metropolitan statistical area", geometry=True, ...) will raise:

ValueError: No TIGER/Line shapefile mapping for geography: 'metropolitan statistical area'

Either map "metropolitan statistical area""cbsa" before calling attach_geometry, or raise a more informative error earlier (or document this limitation).

Fix this →


Documentation Issue

🟡 Stale docstring in estimates.py

pypums/estimates.py:83-84

output
    ``"tidy"`` (default) or ``"wide"``.
    Not yet implementedcurrently returns wide format only.

The implementation is there (lines 181–195) — this note was left over from Phase 3. Should be removed.

Fix this →


Minor / Design Notes

FIPS columns silently dropped in tidy output (estimates.py)

In the tidy path (line 184), geo_set = {"state", "county"} is included in excluded, so raw FIPS columns are excluded from both id_cols and value_vars, and silently disappear from the melted output. The GEOID is preserved, so this is usually fine, but it's worth a comment so future maintainers don't wonder where state/county went.

No test for geometry=True in flows

The PR description mentions "implement geometry=True support for get_flows()", but tests/test_get_flows.py has no test class for this path. The GEOID construction is tested, which is necessary for geometry to work, but the actual geometry=True invocation isn't covered.

get_survey_metadata with empty catalog

If the Census API returns {"dataset": []}, pd.DataFrame([]) produces a DataFrame with no columns. Downstream code accessing df["vintage"] etc. would raise KeyError. Adding a column scaffold (pd.DataFrame(rows, columns=[...])) or a guard would make this more robust.


What's Working Well

  • The double-melt + merge pattern in get_flows tidy output is clean: estimate and MOE columns are melted separately, _moe_var is stripped of the _M suffix to get a common variable key, then merged. This correctly pairs estimates with their MOE without introducing spurious rows.
  • The thin _fetch_data_json / _call_census_api wrapper pattern is consistently applied across all modules, making mocking in tests simple and precise.
  • Test coverage for MOE scaling, GEOID construction, filtering by year, and invalid-input error paths is solid.

- Include output and moe_level in cache keys for flows.py and
  estimates.py to prevent stale cache hits across different formats
- Map 'metropolitan statistical area' to 'cbsa' before calling
  attach_geometry() so geometry=True works for MSA-level flows
- Remove stale 'Not yet implemented' docstring from estimates.py
  output parameter
- Scaffold column names in get_survey_metadata() so empty catalogs
  return a DataFrame with the expected columns

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Mar 7, 2026

Copy link
Copy Markdown

Claude finished @chekos's task in 49s —— View job


Review feedback verification — all fixes confirmed ✅

  • Gather context
  • Verify cache key fixes in flows.py and estimates.py
  • Verify MSA geometry mapping fix in flows.py
  • Verify stale docstring fix in estimates.py

All four issues from the previous review have been properly addressed in commit 781711b:

🟢 Cache key — moe_level and output in flows.py (L165–169)

cache_key = (
    f"flows_{year}_{geography}_{state}_{county}"
    f"_{msa}_{vars_str}_{breakdown_str}"
    f"_{output}_{moe_level}"
)

Both output and moe_level are now part of the cache key. ✅

🟢 Cache key — output in estimates.py (L144–147)

cache_key = (
    f"est_{vintage}_{resolved_product}_{geography}_{state}_{county}"
    f"_{year}_{vars_str}_{breakdown_str}_{output}"
)

output is now included in the cache key. ✅

🟢 MSA geometry mapping in flows.py (L238–245)

if geometry:
    from pypums.spatial import attach_geometry
    geo_name = geography
    if geography == "metropolitan statistical area":
        geo_name = "cbsa"
    df = attach_geometry(df, geo_name, state=state, year=year)

"metropolitan statistical area" is now correctly mapped to "cbsa" before calling attach_geometry. ✅

🟢 Stale docstring in estimates.py (L83–84)

The "Not yet implemented" note has been removed from the output parameter docstring. ✅


The PR looks good to merge. All previously identified bugs are fixed.

@chekos chekos merged commit 1420d5e into main Mar 7, 2026
13 checks passed
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