Skip to content

Scipy 1.8.0 compat: copy private classes into dask/array/stats.py#8694

Merged
jsignell merged 4 commits intodask:mainfrom
jsignell:scipy
Mar 3, 2022
Merged

Scipy 1.8.0 compat: copy private classes into dask/array/stats.py#8694
jsignell merged 4 commits intodask:mainfrom
jsignell:scipy

Conversation

@jsignell
Copy link
Copy Markdown
Member

@jsignell jsignell commented Feb 9, 2022

@jsignell jsignell requested a review from jrbourbeau February 9, 2022 14:23
@github-actions github-actions bot added the array label Feb 9, 2022
@jsignell jsignell mentioned this pull request Feb 9, 2022
2 tasks
@jsignell
Copy link
Copy Markdown
Member Author

jsignell commented Feb 9, 2022

@rgommers can you comment on what you intend for downstream libraries like dask to do?

@rgommers
Copy link
Copy Markdown
Contributor

rgommers commented Feb 9, 2022

Thanks for the ping @jsignell. Your solution in this PR (just reimplementing the namedtuples) looks like the right one to me. These results classes were never intended to be public, and weren't really public either (stats.stats is just a namespace that was always missing underscores). See scipy/scipy#13118 for a discussion on whether these objects should be added to scipy.stats or not.

@jsignell
Copy link
Copy Markdown
Member Author

jsignell commented Feb 9, 2022

Thanks! Ok one more question @rgommers:

Is scipy.stats.linalg.MatrixLinearOperator meant to be public? It looks like it is a possible output of aslinearoperator so it would be surprising to me if it is purposefully private.

@rgommers
Copy link
Copy Markdown
Contributor

rgommers commented Feb 9, 2022

Is scipy.stats.linalg.MatrixLinearOperator meant to be public? It looks like it is a possible output of aslinearoperator so it would be surprising to me if it is purposefully private.

That may be an oversight, but I'm not sure. LinearOperator and aslinearoperator are in the sparse.linalg namespace, MatrixLinearOperator and IdentityOperator are the two non-underscored objects in _interface.py that are not in sparse.linalg. That said, they're also not in the docs and don't have direct test coverage, which is an indication that they weren't meant to be public.

MatrixLinearOperator is a specialization of LinearOperator, so I'm wondering if you have a use case where you need the former and can't use the latter? If so, I'm happy to propose making it public.

@scharlottej13
Copy link
Copy Markdown
Contributor

@jsignell were there more changes you wanted to make here?

@jsignell
Copy link
Copy Markdown
Member Author

jsignell commented Mar 3, 2022

The changes in this PR kind of work, but they brought up a larger question of how the dask-scipy integration should work. That conversation is happening in #8702

@github-actions github-actions bot added the documentation Improve or add to documentation label Mar 3, 2022
@jsignell
Copy link
Copy Markdown
Member Author

jsignell commented Mar 3, 2022

Ok based on #8702 (comment) I have removed all references to LinearOperator so I think this is good to go.

Copy link
Copy Markdown
Contributor

@phobson phobson left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this.

@jsignell jsignell removed the request for review from jrbourbeau March 3, 2022 15:57
@jsignell jsignell added the almost done Work is almost done! label Mar 3, 2022
@jsignell jsignell merged commit a27b437 into dask:main Mar 3, 2022
@jsignell jsignell deleted the scipy branch March 3, 2022 17:09
@jsignell jsignell removed the almost done Work is almost done! label Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

array documentation Improve or add to documentation upstream

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scipy's gmres fails to converge with Dask Array scipy 1.8.0 compatibility

5 participants