Fix index-oriented JSON parsing - #410
Conversation
|
@nitishchauhan002 is attempting to deploy a commit to the yashkewlani2020-gmailcom's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey @nitishchauhan002, your first PR on Fair Code, that's awesome, thank you. This project is about making AI more accountable, and contributions like yours are what keep that work going. We'll review your changes shortly. If you haven't already, give the contributing guide a quick read: it covers how audits are structured and what we look for in a review. And if you're finding Fair Code useful or interesting, a star on the repo genuinely helps more people find it - no pressure, just appreciated. |
|
@yakew7 @ahmdkaml - new PR touching a path you own, please review. |
There was a problem hiding this comment.
🟡 Changes recommended
The new dict-of-dicts heuristic can incorrectly transpose valid columns-oriented JSON with a non-default index, creating a new silent wrong-shape failure mode.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses #405 by updating faircode/loaders_extra.py to avoid silently mis-parsing pandas orient="index" JSON (which otherwise loads transposed under pandas’ default "columns" assumption), and adds a targeted regression test for the index-orient case.
Changes:
- Add shape-based JSON-orientation detection for pandas split-orient and index-orient inputs in
read_table(). - Add a new test case to ensure index-oriented JSON round-trips without transposition.
File summaries
| File | Description |
|---|---|
faircode/loaders_extra.py |
Adds split-orient detection and a new dict-of-dicts heuristic intended to detect index-oriented JSON before falling back to pandas’ default JSON loader. |
tests/test_json_edge_cases.py |
Adds a regression test covering pandas orient="index" JSON round-tripping via read_table(). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new dict-of-dicts heuristic will misclassify orient="columns" JSON with a non-default index as index-oriented, producing the exact transpose the new test is meant to prevent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
faircode/loaders_extra.py:69
- The current index-oriented JSON detection will also match pandas
orient="columns"output when the DataFrame has a non-default index (inner keys like "a", "b"). In that caseread_table()returnspd.read_json(..., orient="index"), which transposes the data and contradicts the new test case below.
if isinstance(parsed, dict) and parsed and all(
isinstance(value, dict) for value in parsed.values()
):
inner_key_sets = [set(row.keys()) for row in parsed.values()]
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new index-oriented JSON heuristic in faircode/loaders_extra.py appears to misclassify orient="columns" dict-of-dicts with non-default indices, causing transposed DataFrames and likely failing the added test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The current non-square dict-of-dicts heuristic can still silently mis-parse wide orient="index" JSON (rows < columns), leaving the original transpose bug in common cases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
|
Reviewed this locally (checked out the branch, ran the suite, and built some cases beyond the two included tests) - I don't think this actually resolves the ambiguity, it just moves where the silent data corruption happens. Two concrete repros: 1. A genuinely No error, no warning - just wrong data. This is the same failure mode the PR is meant to fix, just triggered from the other direction (the "which dimension is larger" heuristic assumes rows > columns, which isn't guaranteed). 2. A genuinely The type-homogeneity tie-break only works when columns/rows differ in type mix (one string column + one int column, which is exactly what both included tests use). When every column is a string - the norm for this kind of data - there's nothing to break the tie and it silently defaults to the wrong orientation. Both new tests pass only because they reuse the same coincidental 2-column, mixed-type, 2-row example, not because the ambiguity is actually resolved. For a fairness-auditing tool, silently transposing/mislabeling demographic columns is worse than the original bug - it can quietly flip which group looks disadvantaged downstream with no signal anything went wrong. Smaller things while I'm in here:
Suggest replacing the heuristic with what the original issue asked for: detect the ambiguous shape and raise a clear "ambiguous JSON orientation, please specify explicitly" error instead of guessing - a loud, wrong-looking failure is much safer here than a silent one. Existing suite + ruff both pass clean on this branch, for what it's worth - the risk is entirely in shapes the current tests don't cover. |
PR #410's heuristic (larger dimension implies index; type-homogeneity tie-break for square cases) still silently mis-parses/transposes data in cases the PR's own tests didn't cover: columns-oriented data wider than it is tall, and square all-string data where there's no type signal to break the tie on. Both orientations serialize to the same dict-of-dicts JSON shape, so there's no reliable way to infer which one a file was written with from shape or types alone. Replace the heuristic with a hard failure: any dict-of-dicts JSON (outside the already-unambiguous split orientation) now raises a clear "ambiguous JSON orientation" error pointing at orient="split", instead of guessing and risking a silently wrong/transposed DataFrame - which for a fairness-auditing tool could flip which group looks disadvantaged with no visible sign anything went wrong. Also: restore the two files' trailing newlines, and drop an unrelated cosmetic blank-line change to get_xlsx_sheet_info that had crept into the same diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n to live numbers CONTRIBUTORS.md was missing nitishchauhan002 (#410, first merged PR) and shauryagangrade (#425, first merged PR) entirely, and AnayDhawan's #412 wasn't reflected in their existing entry - snapshot bumped to cover everything through #425. CHANGELOG.md gets matching "Fixed" entries under the version already shipping (2.2.0), including the ambiguous-JSON follow-up fix from the previous commit. METRICS.md's weekly row and badges, and README's Traction table, updated to live GitHub numbers (stars, forks, contributors, issues closed this week) pulled via `gh`. Countries/social reach left unchanged - no access to site analytics or Instagram/LinkedIn impressions to verify those.
Summary
Fixes incorrect parsing of index-oriented JSON in
loaders_extra.py.Type
Audit checklist
unfair.pyincludes protected attributes and prints the required output formatfair.pyremoves protected attributes and identified proxy variablesrandom_state=42and an 80/20 train/test splitunfair.pngandfair.pngare included as PNG screenshotsDATA.mdis included if the file is too largeREADME.mdincludes the new results row and audit sectionBefore fairness gap:
After fairness gap:
Reduction:
Protected attribute(s):
Proxy variables dropped:
Explainer checklist
explainers/and uses lowercase hyphenated namingREADME.mdwas updatedLinked issue
Closes #405