Skip to content
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

Remove vestigial/mostly unused backends.linalg module #742

Merged
merged 7 commits into from
Sep 16, 2022

Conversation

shadeMe
Copy link
Collaborator

@shadeMe shadeMe commented Aug 10, 2022

The backends.linalg module was introduced back in 2016, presumably as a BLAS-specific implementation of linear algebra functions that could be used in the context of thinc ops. In the current version of thinc, however, this module is almost entirely unused (and untested), where calls to its functions have been replaced by their more performant equivalents in numpy/cupy.

This PR replaces the last remaining references to the module's functions with numpy and BLAS calls and removes it from the project.

Outside of thinc, this module was only used in the transition-based paser code in spaCy. This spaCy PR replaces those references in a similar fashion.

@shadeMe shadeMe added the feat / ops Backends and maths label Aug 10, 2022
@shadeMe shadeMe requested a review from svlandeg August 10, 2022 13:34
@shadeMe shadeMe changed the base branch from master to v9 August 11, 2022 09:26
@shadeMe shadeMe added the 🔜 v9.0 Related to upcoming v9.0 label Aug 11, 2022
@svlandeg svlandeg requested a review from danieldk August 17, 2022 16:23
thinc/backends/cblas.pxd Outdated Show resolved Hide resolved
thinc/backends/numpy_ops.pyx Outdated Show resolved Hide resolved
thinc/backends/numpy_ops.pyx Outdated Show resolved Hide resolved
@svlandeg svlandeg removed their request for review August 31, 2022 13:45
@shadeMe
Copy link
Collaborator Author

shadeMe commented Sep 7, 2022

@explosion-bot please test_slow_gpu

@explosion-bot
Copy link
Collaborator

explosion-bot commented Sep 7, 2022

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/thinc-slow-gpu-tests/builds/30

@shadeMe
Copy link
Collaborator Author

shadeMe commented Sep 9, 2022

@explosion-bot please test_slow_gpu

@explosion-bot
Copy link
Collaborator

explosion-bot commented Sep 9, 2022

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/thinc-slow-gpu-tests/builds/32

Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

LGTM. Could you add a ticket that we should update thinc-apple-ops to add the ?scal functions when we release v9?

@shadeMe
Copy link
Collaborator Author

shadeMe commented Sep 13, 2022

Seems like the test failure on Windows 3.10 is spurious?

@danieldk danieldk merged commit 0366934 into explosion:v9 Sep 16, 2022
@shadeMe shadeMe deleted the refactor/remove-linalg-backend branch September 16, 2022 12:48
shadeMe added a commit to shadeMe/thinc that referenced this pull request Jan 5, 2023
* `CBlas`: Add `sscalv`

* `NumpyOps`: Replace usage of  `.linalg` with `numpy` and `BLAS` calls

* Remove vestigial/mostly unused `backends.linalg` module

* Use BLAS notation for `sscal`, add `dscal`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat / ops Backends and maths 🔜 v9.0 Related to upcoming v9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants