-
Notifications
You must be signed in to change notification settings - Fork 658
Fix IncrementalPCA: handle missing batch_size_ in sparse transform after partial_fit #8010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a2b019b
eaf6a85
666bfa0
451fba8
4171024
527798c
21c53a4
3063ec6
3f31977
71b45b0
873b231
7c25d4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,21 @@ def test_exceptions(): | |
| cuIPCA(n_components=8).fit(X[:, :5]) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("batch_size", [None, 50]) | ||
| def test_partial_fit_then_sparse_transform(batch_size): | ||
| X_dense, _ = make_blobs( | ||
| n_samples=200, n_features=10, random_state=0, dtype="float64" | ||
| ) | ||
| X_sparse = cupyx.scipy.sparse.csr_matrix(X_dense) | ||
|
|
||
| ipca = cuIPCA(n_components=4, batch_size=batch_size) | ||
| for i in range(0, 200, 50): | ||
| ipca.partial_fit(X_dense[i : i + 50]) | ||
|
|
||
| result = ipca.transform(X_sparse) | ||
| assert result.shape == (200, 4) | ||
|
Comment on lines
+147
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| def test_svd_flip(): | ||
| x = cp.array(range(-10, 80)).reshape((9, 10)) | ||
| u, s, v = cp.linalg.svd(x, full_matrices=False) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 973
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 1096
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 1199
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 867
🏁 Script executed:
Repository: 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:
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 1736
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 87
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 1123
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 196
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 39
🏁 Script executed:
Repository: rapidsai/cuml
Length of output: 2777
Use explicit
Nonefallback forbatch_sizeconsistency withfit().Line 432-434 uses
self.batch_size or 5 * n_features, which treatsbatch_size=0as falsy and silently applies the default. This diverges fromfit()'s explicitif self.batch_size is None:check (line 235). The inconsistency matters whenpartial_fit()is called instead offit()(since partial_fit doesn't setbatch_size_), followed by sparsetransform()with an invalidbatch_size=0parameter—it would silently use the default instead of failing consistently.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents