Adding test (and variables) for sparse matrix building logic #475
Conversation
baogorek
left a comment
baogorek
left a comment
There was a problem hiding this comment.
Hi María,
Nice job on this so far. We should be able to get it merged before stand-up. Please see the comments sprinkled throughout. Here is the big one that could reduce the lines of code substantially:
The TestNationalLevelContributions class (lines 466-656) tests something new and valuable - that national-level targets receive contributions from
households across multiple states without geographic bias. This should stay.
However, TestStateLevelValues and TestCrossStateRecalculation substantially overlap with the existing test_same_state.py and test_cross_state.py. Could we
either:
1. Remove the redundant test classes and rely on the existing tests, or
2. If the new tests add coverage the existing tests miss (e.g., testing more variables), consolidate by extending the existing test files rather than
duplicating the approach?
This would reduce the file from ~875 lines to ~300 lines (just configuration, fixtures, helpers, basic structure tests, and TestNationalLevelContributions).
Feel free to get a second opinion on that.
I was puzzled at first by the number of files in the PR but the black "2026 stable style" was released 4 days ago, so your PR gets to be the one to bring in those changes!
| "snap", | ||
| ] | ||
|
|
||
| # Complications: |
There was a problem hiding this comment.
It's probably better to put these scratchpad comments in an Issue.
| ("unemployment_compensation", 1e-2), | ||
| ] | ||
|
|
||
| # Combined filter config to build matrix with all variables at once |
There was a problem hiding this comment.
Perhaps mention that these come from our policy_data.db
| # ============================================================================= | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") |
There was a problem hiding this comment.
From lines 116 to 170, Claude is telling me that these are "duplicate fixtures" that already exist in conftest.py. Please double check on that.
The file redefines db_uri, dataset_path, sim, builder, test_cds which already exist in conftest.py. This isn't strictly an "artifact" but it's unnecessary
duplication that makes the file longer.
| mismatch_df = pd.DataFrame(all_mismatches) | ||
| print(mismatch_df.head(15).to_string()) | ||
|
|
||
| mismatch_df.to_csv("state_level_mismatches.csv", index=False) |
There was a problem hiding this comment.
Claude is suggesting to drop the CSV to keep the CI/CD environment cleaner, allowing the assert message to do the hard work there.
| # ============================================================================= | ||
|
|
||
|
|
||
| class TestStateLevelValues: |
There was a problem hiding this comment.
See the overall review message for comment about potential overlap with test_same_state.py.
| # ============================================================================= | ||
|
|
||
|
|
||
| class TestCrossStateRecalculation: |
There was a problem hiding this comment.
See overall review message for a question about whether this is overlapping with test_cross_state.py.
Fix #472