Add include_bias option to Polynomial basis function#230
Draft
kimjune01 wants to merge 3 commits into
Draft
Conversation
Implements the feature requested in issue wilsonrljr#193 to allow users to optionally include a bias (intercept) term in the Polynomial basis function, matching the interface already present in newer basis functions like Legendre. Changes: - Add `include_bias` parameter to Polynomial.__init__ (default=False for backward compatibility) - Modify fit() to prepend a column of ones when include_bias=True - Handle predefined_regressors correctly when bias is included - Support both NumPy and Array API backends Tests: - Add comprehensive test suite in test_polynomial.py - Verify backward compatibility with existing behavior - Test bias inclusion/exclusion - Test interaction with predefined_regressors - Test shape changes Maintains backward compatibility: default behavior unchanged.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 6 high |
🟢 Metrics 10 complexity · 0 duplication
Metric Results Complexity 10 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Fixes four critical bugs identified in code review: 1. Double bias bug: build_lagged_matrix already prepends a bias column at index 0. The previous implementation added another bias column, creating rank deficiency. Now we strip the existing bias column before polynomial expansion, then optionally re-add it. 2. include_bias=False now actually excludes bias: Previously the (0,0,...,0) combination still generated a constant term even when include_bias=False. Now we remove the bias column before expansion, so all-zero combinations cannot occur. 3. regressor_space index mapping: By stripping the bias column first and optionally re-adding it after expansion, regressor indices now map correctly whether include_bias is True or False. 4. Default consistency: Changed default from False to True to match Legendre basis function behavior. Implementation follows Legendre's pattern: - Remove bias column (index 0) from build_lagged_matrix output - Generate polynomial terms from remaining features - Re-add bias column if include_bias=True - Apply predefined_regressors filtering after bias handling
Prior commit broke backward compatibility: with include_bias=True the output lost the cross-bias polynomial terms (combinations like (0,k)) because fit() sliced column 0 from the input before polynomial expansion. test_fit_polynomial in test_basis_functions.py caught this. Restore the original fit() path on default. When include_bias=False, drop the (0,0,...,0) pure-bias column emitted first by combinations_with_replacement, and drop the matching row from RegressorDictionary.regressor_space so regressor_code stays aligned with psi. Per maintainer guidance on PR wilsonrljr#197.
Author
|
Cannot validate on current setup, so drafting. Please close or take it over. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Legendre basis function supports include_bias for controlling whether a bias column is included, but the Polynomial basis function doesn't. Users who need to exclude the bias term from polynomial expansion have no way to do so without manual post-processing.
This adds the same include_bias parameter to Polynomial, following the existing pattern in the Legendre implementation. Default is False to maintain backward compatibility. Includes Array API support for all backends (NumPy, PyTorch, CuPy, JAX).
Fixes #193