Fix IncrementalPCA: handle missing batch_size_ in sparse transform after partial_fit - #8010
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
55b56ed to
873b231
Compare
csadorf
changed the title
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe change modifies Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/decomposition/incremental_pca.py`:
- Around line 432-434: Replace the current truthy fallback that uses
"self.batch_size or 5 * n_features" with an explicit None-check so a value of 0
is preserved; get batch_size via getattr(self, "batch_size_", None) and then if
batch_size is None set batch_size = self.batch_size if self.batch_size is not
None else 5 * n_features (so behavior matches fit()'s "if self.batch_size is
None" logic and partial_fit/transform handle batch_size=0 consistently).
Reference: attribute names batch_size_ and batch_size and methods partial_fit,
fit, transform.
In `@python/cuml/tests/test_incremental_pca.py`:
- Around line 147-148: The test only checks shape after calling
ipca.transform(X_sparse); update it to also compute the dense transform (e.g.,
dense_result = ipca.transform(X)) and assert numerical closeness between the
sparse and dense transform outputs using numpy.testing.assert_allclose with
atol=5e-5 (or equivalent) to catch value regressions; reference ipca.transform,
X_sparse, X and the result variables when adding the comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d945c8b9-e8b1-41ee-a301-05d79d275d68
📒 Files selected for processing (2)
python/cuml/cuml/decomposition/incremental_pca.pypython/cuml/tests/test_incremental_pca.py
| batch_size = getattr( | ||
| self, "batch_size_", self.batch_size or 5 * n_features | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
cat -n python/cuml/cuml/decomposition/incremental_pca.py | sed -n '425,445p'Repository: rapidsai/cuml
Length of output: 973
🏁 Script executed:
# Search for how fit() handles batch_size
rg -n "def fit" python/cuml/cuml/decomposition/incremental_pca.py -A 30 | head -60Repository: rapidsai/cuml
Length of output: 1096
🏁 Script executed:
# Check for batch_size handling in the codebase
rg -n "batch_size" python/cuml/cuml/decomposition/incremental_pca.py | head -20Repository: rapidsai/cuml
Length of output: 1199
🏁 Script executed:
# Check __init__ method and any parameter validation
sed -n '170,200p' python/cuml/cuml/decomposition/incremental_pca.pyRepository: rapidsai/cuml
Length of output: 867
🏁 Script executed:
# Search for any validation of batch_size in the file
rg -n "batch_size" python/cuml/cuml/decomposition/incremental_pca.py -B 2 -A 2 | grep -E "(validate|check|assert|raise|< |> |== 0)" | head -20Repository: rapidsai/cuml
Length of output: 401
🌐 Web query:
scikit-learn IncrementalPCA batch_size parameter documentation behavior
💡 Result:
In scikit-learn’s IncrementalPCA, the batch_size parameter only affects how data are split into minibatches when you call fit. It is not used by partial_fit. 1) What the official docs say about batch_size - batch_size is “Only used when calling fit.” - If batch_size=None, it is inferred from the data and set to 5 * n_features. These statements are in the IncrementalPCA class reference docs. 2) What happens to batch_size under the hood (fit) - In IncrementalPCA.fit(X), scikit-learn sets an internal attribute batch_size_: - if self.batch_size is None: batch_size_ = 5 * n_features - else: batch_size_ = self.batch_size - Then fit iterates over the training data in minibatches generated using batch_size_ and calls partial_fit on each batch. 3) How partial_fit relates to batch_size - The docs explicitly limit batch_size usage to fit, implying partial_fit does not consult the batch_size parameter. - partial_fit receives whatever X you pass to it; batching is your responsibility when calling partial_fit directly. 4) Practical implication / typical usage patterns - If you want automatic batching, call ipca.fit(X, ...) with batch_size set. - If you want to stream/choose your own batch sizes, ignore batch_size and call ipca.partial_fit(X_batch) with batches you create yourself. Example behavior from the docs: - With IncrementalPCA(n_components=7, batch_size=200), calling transformer.partial_fit(X[:100, :]) keeps batch_size=200 shown on the estimator object, but the underlying batch granularity comes from the array you passed to partial_fit (not from the estimator’s batch_size parameter).
Citations:
- 1: https://scikit-learn.org/stable/modules/generated/sklearn.decomposition.IncrementalPCA.html
- 2: https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/decomposition/_incremental_pca.py
🏁 Script executed:
# Check if transform() is supposed to batch or if this is specific to sparse handling
sed -n '400,445p' python/cuml/cuml/decomposition/incremental_pca.pyRepository: rapidsai/cuml
Length of output: 1736
🏁 Script executed:
# Look for parent class and see if there's a note about sparse vs dense behavior
grep -n "class IncrementalPCA" python/cuml/cuml/decomposition/incremental_pca.pyRepository: rapidsai/cuml
Length of output: 87
🏁 Script executed:
# Check if there's any documentation about batching in transform
sed -n '380,410p' python/cuml/cuml/decomposition/incremental_pca.pyRepository: rapidsai/cuml
Length of output: 1123
🏁 Script executed:
# Search for any validation of batch_size parameter value (not just None checks)
rg -n "batch_size" python/cuml/cuml/decomposition/incremental_pca.py | grep -E "(validate|error|raise|assert|<|>|>=|<=)"Repository: rapidsai/cuml
Length of output: 196
🏁 Script executed:
# Check if there's parameter validation in parent class or base module
grep -r "batch_size" python/cuml/cuml/decomposition/ --include="*.py" | grep -E "(validate|check|raise|assert)" | head -10Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
# Look for partial_fit to see if it also sets batch_size_
sed -n '300,370p' python/cuml/cuml/decomposition/incremental_pca.pyRepository: rapidsai/cuml
Length of output: 2777
Use explicit None fallback for batch_size consistency with fit().
Line 432-434 uses self.batch_size or 5 * n_features, which treats batch_size=0 as falsy and silently applies the default. This diverges from fit()'s explicit if self.batch_size is None: check (line 235). The inconsistency matters when partial_fit() is called instead of fit() (since partial_fit doesn't set batch_size_), followed by sparse transform() with an invalid batch_size=0 parameter—it would silently use the default instead of failing consistently.
Suggested fix
- batch_size = getattr(
- self, "batch_size_", self.batch_size or 5 * n_features
- )
+ batch_size = getattr(
+ self,
+ "batch_size_",
+ self.batch_size
+ if self.batch_size is not None
+ else 5 * n_features,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| batch_size = getattr( | |
| self, "batch_size_", self.batch_size or 5 * n_features | |
| ) | |
| batch_size = getattr( | |
| self, | |
| "batch_size_", | |
| self.batch_size | |
| if self.batch_size is not None | |
| else 5 * n_features, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuml/cuml/decomposition/incremental_pca.py` around lines 432 - 434,
Replace the current truthy fallback that uses "self.batch_size or 5 *
n_features" with an explicit None-check so a value of 0 is preserved; get
batch_size via getattr(self, "batch_size_", None) and then if batch_size is None
set batch_size = self.batch_size if self.batch_size is not None else 5 *
n_features (so behavior matches fit()'s "if self.batch_size is None" logic and
partial_fit/transform handle batch_size=0 consistently). Reference: attribute
names batch_size_ and batch_size and methods partial_fit, fit, transform.
| result = ipca.transform(X_sparse) | ||
| assert result.shape == (200, 4) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file at the specified lines
cd python/cuml/tests && head -160 test_incremental_pca.py | tail -40Repository: rapidsai/cuml
Length of output: 1301
🏁 Script executed:
# Also get a broader context around the test
cat -n python/cuml/tests/test_incremental_pca.py | sed -n '130,160p'Repository: rapidsai/cuml
Length of output: 1300
🏁 Script executed:
# Check what imports and fixtures are available in the test file
head -50 python/cuml/tests/test_incremental_pca.pyRepository: rapidsai/cuml
Length of output: 1467
🏁 Script executed:
# Search for other tests in the file that test sparse transforms
rg "sparse.*transform|transform.*sparse" python/cuml/tests/test_incremental_pca.py -B 2 -A 5Repository: rapidsai/cuml
Length of output: 522
🏁 Script executed:
# Check for assert_allclose or similar numerical comparison patterns in the test file
rg "assert_allclose|array_equal|np.allclose" python/cuml/tests/test_incremental_pca.py -B 1 -A 1Repository: rapidsai/cuml
Length of output: 369
🏁 Script executed:
# Look at test_fit to see how it validates results with sparse input
cat -n python/cuml/tests/test_incremental_pca.py | sed -n '10,100p'Repository: rapidsai/cuml
Length of output: 3302
🏁 Script executed:
# Search for all assertions comparing cuml results to sklearn results in this file
rg "assert_allclose|array_equal|skIPCA|sk_" python/cuml/tests/test_incremental_pca.py -B 2 -A 1Repository: rapidsai/cuml
Length of output: 1138
🏁 Script executed:
# Check if there are any other tests combining partial_fit + transform
rg "partial_fit.*transform|transform.*partial_fit" python/cuml/tests/test_incremental_pca.py -B 3 -A 3Repository: rapidsai/cuml
Length of output: 271
🏁 Script executed:
# Check the array_equal function signature to verify tolerance syntax
rg "def array_equal" --type py -A 5Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
# Look at test_partial_fit more completely to see the full sklearn comparison pattern
cat -n python/cuml/tests/test_incremental_pca.py | sed -n '88,120p'Repository: rapidsai/cuml
Length of output: 1224
🏁 Script executed:
# Find the array_equal utility function signature
fd "testing" --type f | xargs grep -l "def array_equal" 2>/dev/null | head -5Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
# Search for array_equal definition
rg "def array_equal" -A 8Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
# Find the testing utils module
find python/cuml -name "*testing*" -type f | head -10Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
# Search for the testing.utils file
find . -path "*/testing/utils.py" -o -path "*/testing/utils/__init__.py"Repository: rapidsai/cuml
Length of output: 93
🏁 Script executed:
# Check the array_equal function signature and documentation
head -200 python/cuml/cuml/testing/utils.py | grep -A 20 "def array_equal"Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
# Search for array_equal in the utils file
cat python/cuml/cuml/testing/utils.py | grep -A 15 "def array_equal"Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
# Let's just read the testing utils file
wc -l python/cuml/cuml/testing/utils.pyRepository: rapidsai/cuml
Length of output: 95
🏁 Script executed:
# Read a relevant portion of the file
head -100 python/cuml/cuml/testing/utils.pyRepository: rapidsai/cuml
Length of output: 3004
🏁 Script executed:
# Let's verify that the test can indeed call transform with dense data on the same fitted model
cat -n python/cuml/tests/test_incremental_pca.py | sed -n '145,160p'Repository: rapidsai/cuml
Length of output: 713
🏁 Script executed:
# Check if there are any tests comparing sparse vs dense transforms elsewhere
rg "transform.*sparse|sparse.*transform" python/cuml/tests/test_incremental_pca.py -B 5 -A 5Repository: rapidsai/cuml
Length of output: 690
🏁 Script executed:
# Look at test_fit to see full pattern of how sklearn comparison is done
cat -n python/cuml/tests/test_incremental_pca.py | sed -n '31,75p'Repository: rapidsai/cuml
Length of output: 1413
🏁 Script executed:
# Check if there's a specific convention for transform result tolerance
rg "transform.*assert|assert.*transform" python/cuml/tests/test_incremental_pca.py -B 2 -A 2Repository: rapidsai/cuml
Length of output: 39
Add numerical assertions to catch value regressions alongside shape validation.
The test currently only validates shape but not correctness of the transformed values. Per coding guidelines, tests must validate numerical correctness. Add a comparison of sparse vs dense transform paths to ensure the sparse path produces correct results:
Suggested enhancement
result = ipca.transform(X_sparse)
+ dense_result = ipca.transform(X_dense)
assert result.shape == (200, 4)
+ assert array_equal(result, dense_result, 5e-5)(Using 5e-5 tolerance to align with the codebase convention established in test_fit and test_partial_fit.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuml/tests/test_incremental_pca.py` around lines 147 - 148, The test
only checks shape after calling ipca.transform(X_sparse); update it to also
compute the dense transform (e.g., dense_result = ipca.transform(X)) and assert
numerical closeness between the sparse and dense transform outputs using
numpy.testing.assert_allclose with atol=5e-5 (or equivalent) to catch value
regressions; reference ipca.transform, X_sparse, X and the result variables when
adding the comparison.
|
/merge |
4 participants
Previously
transformon sparse inputs accessedself.batch_size_directly, which is only set byfit, notpartial_fit. Callingtransformafterpartial_fitraised anAttributeError.Fall back to
self.batch_size(or5 * n_featuresif unset) whenbatch_size_is not present, matching the behavior used infit.Follow-up to #8006