Skip to content

[ENH] unit tests for gen_imgs.py#396

Merged
fkiraly merged 5 commits into
dswah:mainfrom
Yasserelhaddar:387-gen_imgs-tests
Aug 10, 2025
Merged

[ENH] unit tests for gen_imgs.py#396
fkiraly merged 5 commits into
dswah:mainfrom
Yasserelhaddar:387-gen_imgs-tests

Conversation

@Yasserelhaddar

@Yasserelhaddar Yasserelhaddar commented Jul 25, 2025

Copy link
Copy Markdown
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.

@Yasserelhaddar Yasserelhaddar mentioned this pull request Jul 25, 2025
Closed
@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
fkiraly
fkiraly requested changes Jul 26, 2025
View reviewed changes

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

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

Yasserelhaddar commented Jul 26, 2025

Copy link
Copy Markdown
Contributor Author

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

fkiraly
fkiraly requested changes Jul 29, 2025
View reviewed changes

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

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?

dswah
dswah approved these changes Jul 30, 2025
View reviewed changes

@dswah dswah left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

thanks @Yasserelhaddar !

Comment thread gen_imgs.py
Comment thread gen_imgs.py
Comment thread pygam/tests/test_gen_imgs.py
@dswah

dswah commented Jul 30, 2025

Copy link
Copy Markdown
Owner

@fkiraly do you see anything missing?

@Yasserelhaddar
Move test_gen_imgs.py to root tests/ directory per maintainer feedback
f564f28 
- 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

Yasserelhaddar commented Jul 31, 2025

Copy link
Copy Markdown
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 ?

fkiraly
fkiraly approved these changes Aug 10, 2025
View reviewed changes

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

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

fkiraly commented Aug 10, 2025

Copy link
Copy Markdown
Collaborator

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

Reviewers

@fkiraly fkiraly fkiraly approved these changes

@dswah dswah dswah approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ENH] test coverage for gen_imgs

4 participants

@Yasserelhaddar @dswah @fkiraly @YasserElHaddar16