Skip to content

[BUG] Replace bare print with warning on non-convergence, fix ValueError format#571

Open
tarun-227 wants to merge 2 commits intodswah:mainfrom
tarun-227:fix/convergence-warning-and-format-bug
Open

[BUG] Replace bare print with warning on non-convergence, fix ValueError format#571
tarun-227 wants to merge 2 commits intodswah:mainfrom
tarun-227:fix/convergence-warning-and-format-bug

Conversation

@tarun-227
Copy link
Copy Markdown

@tarun-227 tarun-227 commented Mar 31, 2026

Summary

Two bugs found by code review:

  1. print("did not converge")warnings.warn() (pygam.py:821)

    • Libraries should not use print() for user-facing messages. Replaced with warnings.warn() so users can programmatically capture/filter non-convergence events via Python's warning system.
  2. Malformed ValueError in _estimate_GCV_UBRE (pygam.py:1196-1199)

    • format(gamma) was passed as a separate argument to ValueError instead of being interpolated into the string. This produced a confusing tuple error message:
      # Before (broken):
      ValueError('gamma scaling should be greater than 1, but found gamma = {}', '0.5')
      
      # After (fixed):
      ValueError('gamma scaling should be greater than 1, but found gamma = 0.5')

Test plan

  • Added test_non_convergence_emits_warning — verifies warning is emitted (not printed)
  • Added test_GCV_UBRE_gamma_error_message — verifies error message contains the actual gamma value
  • All 164 tests pass (162 existing + 2 new)

- Replace `print("did not converge")` with `warnings.warn()` so that
  non-convergence is surfaced through Python's warning system instead of
  printing to stdout. This allows users to filter/capture the message
  programmatically.

- Fix string formatting bug in `_estimate_GCV_UBRE` where `format(gamma)`
  was passed as a separate argument to `ValueError` instead of being
  interpolated into the message string, resulting in a confusing tuple
  error message like `('msg {}', '0.5')`.
Copy link
Copy Markdown

@Tushar8466 Tushar8466 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [MNT] fix ruff-format commit (3ce140a) is failing —
the ruff formatting check is not passing.

You may need to run ruff locally and fix the formatting before this can be merged:

ruff format pygam/
ruff check pygam/ --fix
git add .
git commit -m "fix: ruff formatting"
git push origin fix/convergence-warning-and-format-bug

Once that's resolved, this looks good to merge! ✅

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