Skip to content

Conversation

@Yasserelhaddar
Copy link
Contributor

Add Unit Tests for gen_imgs.py (Closes #387)

Summary

This PR addresses Issue #387 by adding comprehensive unit test coverage for the gen_imgs.py module, which generates visualization plots for pyGAM documentation.

Changes Made

Bug Fixes

  • Fixed API compatibility issues in gen_imgs.py:
    • Updated gam.generate_X_grid() calls to include required term parameter
    • Fixed gam.partial_dependence() calls to use correct term and X arguments
    • Added robust lambda value extraction for gam.lam to handle nested arrays/lists

New Tests

  • Added test_gen_imgs.py with unit tests for all plotting functions:
    • test_gen_basis_fns() - Tests basis function visualization
    • test_cake_data_in_one() - Tests cake data plotting
    • test_faithful_data_poisson() - Tests faithful data with Poisson distribution
    • test_single_data_linear() - Tests single data linear model
    • test_mcycle_data_linear() - Tests motorcycle data (2 plots)
    • test_wage_data_linear() - Tests wage data visualization
    • test_default_data_logistic() - Tests logistic regression
    • test_constraints() - Tests constraint visualization
    • test_trees_data_custom() - Tests custom tree data
    • test_gen_multi_data() - Tests multi-data generation (2 plots)
    • test_gen_tensor_data() - Tests tensor data visualization
    • test_chicago_tensor() - Tests Chicago tensor data
    • test_expectiles() - Tests expectile regression

Testing Strategy

  • Unit Tests Only: Following pyGAM's existing test philosophy
  • Mocked Dependencies: Uses mock.patch to isolate plt.savefig calls
  • No File I/O: Prevents actual image generation during tests
  • Fast Execution: Tests run quickly without complex model fitting
  • Comprehensive Coverage: Tests all functions that generate plots

Test Results

>> pytest pyGAM/pygam/tests/test_gen_imgs.py -v
============================================= test session starts ==============================================
platform darwin -- Python 3.10.18, pytest-7.4.4, pluggy-1.6.0
rootdir: /Users/yasserelhaddar/opensource_contributions/pyGAM
plugins: cov-4.1.0
collected 13 items                                                                                             

pygam/tests/test_gen_imgs.py .............                                                               [100%]

======================================== 13 passed in 4.36s =========================================

Files Changed

  • pyGAM/gen_imgs.py - API compatibility fixes
  • pyGAM/pygam/tests/test_gen_imgs.py - New test file (created)

Why This Approach?

  • Aligns with pyGAM conventions: Uses from mock import patch like other test files
  • Maintains test isolation: No side effects or file system operations
  • Follows existing patterns: Matches docstring style and test structure of other pyGAM tests
  • Future-proof: Easy to extend with additional test cases

Related Issue

Closes #387 - "Add test coverage for gen_imgs.py"


Note: The API fixes in gen_imgs.py were necessary because the script was using outdated pyGAM method signatures. These fixes ensure the script works with the current pyGAM version while maintaining backward compatibility.

@fkiraly fkiraly changed the title 387 gen imgs tests [MNT] unit tests for gen_imgs.py Jul 26, 2025
@fkiraly fkiraly changed the title [MNT] unit tests for gen_imgs.py [ENH] unit tests for gen_imgs.py Jul 26, 2025
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Very quickly before I review: the file has conflicts with the file on main, and is not passing code formatting checks. Please ensure you have checked with pre-commit.

@Yasserelhaddar
Copy link
Contributor Author

Hey @fkiraly, Thanks for the feedback.
Both concerns are addressed.
Thank you !

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Great! @dswah might like to review if this makes sense logic-wise.

Regarding folder structure, I think this might not exactly work - and it will have a broken import inside the package, since the root files are not packaged.

Could you put the test outside the package and ensure it is run there?

Copy link
Owner

@dswah dswah left a comment

Choose a reason for hiding this comment

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

thanks @Yasserelhaddar !

@dswah
Copy link
Owner

dswah commented Jul 30, 2025

@fkiraly do you see anything missing?

- Moved test file from pygam/tests/ to tests/ to align with gen_imgs.py being in root
- Verified pytest auto-discovers and runs tests from new location
- All 13 tests pass successfully
- Addresses maintainer feedback on folder structure
@Yasserelhaddar
Copy link
Contributor Author

@dswah thanks for the feedback.
@fkiraly, I have restructured the test to be in root/test folder. They are also discoverable in the ci workflow.
Note: out of curiosity I ran the pre-commit hooks across all the files and I got over 800 Errors and linting inconsistencies. Maybe this needs your attention ?

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

No, all good from my side!

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 10, 2025

Note: out of curiosity I ran the pre-commit hooks across all the files and I got over 800 Errors and linting inconsistencies. Maybe this needs your attention ?

We only added pre-commit recently, so many of the files do not adhere to the stricter formatting. Fixing this would of course be appreciated, @Yasserelhaddar!

@fkiraly fkiraly merged commit 9b9ff73 into dswah:main Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENH] test coverage for gen_imgs

4 participants