Skip to content

Address remaining code review comments from PR #159#165

Merged
crvernon merged 3 commits intodevfrom
copilot/sub-pr-159-please-work
Nov 17, 2025
Merged

Address remaining code review comments from PR #159#165
crvernon merged 3 commits intodevfrom
copilot/sub-pr-159-please-work

Conversation

Copy link
Copy Markdown

Copilot AI commented Nov 17, 2025

Addresses the remaining code review feedback from PR #159 that wasn't covered by PRs #160-164. Fixes include removing unused parameters, improving test robustness, and adding data validation.

API cleanup

  • Removed unused base parameter from plot_contour_map function signature and all call sites (utils.py, tests, notebook)
  • Removed unused ResultsWrapper import from test file

Robustness improvements

  • Added data trimming before reshape operations to handle non-divisible-by-10 data lengths:
    # Before: raises ValueError if len(data) not divisible by 10
    xpoints = np.mean(dta[xvar].values.reshape(-1, 10), axis=1)
    
    # After: trims to nearest multiple of 10
    n_trim = (len(dta[xvar].values) // 10) * 10
    xpoints = np.mean(dta[xvar].values[:n_trim].reshape(-1, 10), axis=1)

Test improvements

  • Removed unreachable code in test_empty_data (lines within always-false conditional)
  • Fixed test_logit_with_interaction to avoid mutating shared fixture data
  • Replaced brittle assertions:
    • Removed assert np.all(np.abs(result.params) > 1e-5) (too strict for legitimate near-zero coefficients)
    • Replaced exact equality check with property-based validation (verify predictors present vs. hardcoded coefficient values)
    • Removed non-deterministic p-value significance assertion dependent on random data

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.20%. Comparing base (4071d0e) to head (a6c6aaf).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #165      +/-   ##
==========================================
+ Coverage   94.01%   94.20%   +0.19%     
==========================================
  Files          13       13              
  Lines         935      915      -20     
==========================================
- Hits          879      862      -17     
+ Misses         56       53       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: crvernon <3947069+crvernon@users.noreply.github.com>
Copilot AI changed the title [WIP] Add testing and improvements for plotting utilities Address remaining code review comments from PR #159 Nov 17, 2025
Copilot AI requested a review from crvernon November 17, 2025 21:32
@crvernon crvernon marked this pull request as ready for review November 17, 2025 21:39
@crvernon crvernon merged commit f3458d5 into dev Nov 17, 2025
6 checks passed
@crvernon crvernon deleted the copilot/sub-pr-159-please-work branch November 17, 2025 22:28
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.

2 participants