8000 TST Extend tests for `scipy.sparse.*array` in `test_glm.py` by ivirshup · Pull Request #27107 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

TST Extend tests for scipy.sparse.*array in test_glm.py #27107

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

Closed

Conversation

ivirshup
Copy link
Contributor

Reference Issues/PRs

Towards #27090

What does this implement/fix? Explain your changes.

Adds tests for csr_matrix and csr_array to test_glm.py

Any other comments?

Warning
I think this probably shouldn't be merged.

This more than triples the run time of test_glm.py and is probably more extensive than is actually needed for this case.

But I would appreciate guidance on the correct way to implement this. AFAICT there are no existing tests for sparse input to GLMs, even though they seem to work.

@github-actions
Copy link

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: f681bc4. Link to the linter CI: here

@ivirshup
Copy link
Contributor Author

@jjerphan what's a reasonable time increase for these tests? Also, am I missing any existing tests for sparse matrices here?

@jjerphan
Copy link
Member

what's a reasonable time increase for these tests?

Thanks for pointing this out. I would suggest not worrying much about this: we can add a setup of tests to only have *_CONTAINERS contain sparse matrices' class for most CI runs.

As for your questions:

AFAICT there are no existing tests for sparse input to GLMs, even though they seem to work.

Also, am I missing any existing tests for sparse matrices here?

Code coverage does the absence of tests cases for sparse data with GLMs. Yet, adding more tests for GLM on sparse data would be appreciated.

I do not have the bandwidth to look at that for now, but I might later.

Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Maybe we could introduce safe_sparse_hstack / safe_sparse_vstack next to our safe_sparse_dot.

I am not particularly found of the safe_ prefix but we could reuse it for the sake of consistency.

@@ -270,7 +273,10 @@ def test_glm_regression_hstacked_X(solver, fit_intercept, glm_dataset):

model = clone(model).set_params(**params)
X = X[:, :-1] # remove intercept
X = 0.5 * np.concatenate((X, X), axis=1)
if sparse.issparse(X):
X = np.multiply(sparse.hstack((X, X)), 0.5)
Copy link
Member

Choose a reason for hiding this comment

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

The following looks simpler:

Suggested change
X = np. 8000 multiply(sparse.hstack((X, X)), 0.5)
X = 0.5 * sparse.hstack((X, X))

@ogrisel
Copy link
Member
ogrisel commented Aug 21, 2023

This more than triples the run time of test_glm.py and is probably more extensive than is actually needed for this case.

I am not sure if this is worth it or not. For most solvers, the numerical code is probably independent to the original data sparsity pattern. But there might be solvers (e.g. SAG / SAGA) who are actually specialized for sparse input datastructures (different solver branches).

I have no strong opinion one way or another myself.

Any opinion @lorentzenchr?

@lorentzenchr
Copy link
Member

This more than triples the run time of test_glm.py and is probably more extensive than is actually needed for this case.

I would definitely NOT extend glm_dataset by sparse arrays in the proposed way. It would be enough to have one single test (maybe with one glm_dataset) and parametrized by a few things like sample weights that tests for same coefficients with sparse and dense fitting.

@ogrisel
Copy link
Member
ogrisel commented Aug 24, 2023

Alright. Let's keep test_glm.py unchanged then and instead make sure that scipy sparse array support works by updating test_logistic.py, test_ridge.py and co.

@ogrisel ogrisel closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0