Repository navigation
feat: support complex-valued dask-backed arrays in SVD decomposition - #252
Open
philipdkennedy wants to merge 3 commits into
Open
philipdkennedy wants to merge 3 commits into
philipdkennedy wants to merge 3 commits into
Conversation
Contributor
|
@philipdkennedy thanks for the PR! I'll try to review it within the next day – please bear with me a little. |
Contributor
|
Sorry @philipdkennedy that I keep you waiting for so long. The CI requires some updates that I'm working on, then we can bring in your changes cleanly. I'll keep you updated. |
Contributor
|
Hi again @philipdkennedy – once #257 is merged, could you update your branch by pulling the latest changes. The CI should restart automatically and many (hopefully all?) failing tests will be gone. |
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
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.
Description
Complex-valued, dask-backed arrays can now be decomposed. Previously, the dispatch
matrix in the
Decomposerhad no branch for the complex + dask combination, soHilbert EOF/MCA on chunked data raised — the restriction existed because the code
path was never implemented, not because the numerics are unsound.
xeofs/linalg/decomposer.pyandxeofs/linalg/_numpy/_svd.py: the dask branchnow catches complex-valued input alongside real-valued input, for both the
full(tsqr) andrandomized(svd_compressed) solvers.assert_not_complexin GWPCA, SparsePCA, OPA, MultiCCA)are intentionally retained — those algorithms do not support complex input
and should continue to reject it loudly.
solver="full"on dask input requires tall-and-skinny chunking(single-column-of-blocks), as before; documented in the test fixtures.
Behavioural change
HilbertEOF.test_computeexpectations inverted: fitting on dask inputpreviously raised
NotImplementedError; it now succeeds and returnscomplex-valued components and scores. This is the intended capability.
Tests
tests/linalg/test_decomposer.py:test_fit_dask_full_complex,test_fit_dask_randomized_complex,test_dask_array_based_on_target_variance_complex, and adask_complexcase in the
test_random_stateparametrization.main, thecomplex-dask tests fail; with this branch, 29/29 pass in
tests/linalg/and the full suite is green apart from two pre-existing
test_mca.pyfailures that also occur on
main.mock_complex_data_array/mock_dask_complex_data_arrayin
tests/linalg/test_decomposer.py, mirroring the real-data fixtures.Dependency note
Complex-valued dask input requires dask >= 2026.6.0: earlier versions
contained conjugation bugs in
svd_compressed/tsqrfor complex data(dask/dask#7639, dask/dask#9421), producing silently incorrect
decompositions. Verified against 2026.6.0, where the numerics are correct
(round-trip tested in
tests/linalg/).Known issue
Complex-dask fits emit a cosmetic
ComplexWarningfrom dask's metaconstruction (hardcoded
float64meta intsqr, upstreamdask/dask#10284). It is numerically inert since dask's complex conjugation
fixes. Suppressed locally in the affected tests with a comment, rather than
project-wide, so a future genuine cast is not hidden.
Agreed in advance with @nicrie to fix #197.