Skip to content

Remove LazyArrays.jl dependency #750

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ChrisRackauckas-Claude
Copy link
Contributor

Summary

This PR removes the LazyArrays.jl dependency from LinearSolve.jl, simplifying the codebase as suggested.

Changes

  • Removed LazyArrays from Project.toml dependencies (both main dependency and compat entry)
  • Replaced BroadcastArray(@~ .-(λ .* tu)) with simple broadcast .-(λ .* tu) in src/adjoint.jl:87
  • Removed LazyArrays import from src/LinearSolve.jl
  • Updated tests in test/adjoint.jl to check for AbstractMatrix instead of BroadcastArray
  • Removed LazyArrays import from test file

Rationale

The LazyArrays dependency was only used in one place (src/adjoint.jl:87) to create a lazy broadcast array. The regular Julia broadcast operation is sufficient for this use case and doesn't require the additional dependency. This simplification reduces the dependency footprint of the package without any loss of functionality.

Test Results

All adjoint tests pass successfully with the simplified implementation.

🤖 Generated with Claude Code

- Removed LazyArrays from Project.toml dependencies
- Replaced BroadcastArray(@~ .-(λ .* tu)) with simple broadcast .-(λ .* tu) in adjoint.jl
- Updated tests to check for AbstractMatrix instead of BroadcastArray
- All tests pass successfully
@ChrisRackauckas
Copy link
Member

@avik-pal I assume this is overkill if Reactant is used? Because it brings in some unwanted dependencies so we kind of need to get rid of it.

@ChrisRackauckas
Copy link
Member

Or we can make it an extension if it's a very important optimization that isn't covered by Reactant.

@avik-pal
Copy link
Member

@ChrisRackauckas can you remind me what particular optimization this was added for? If it was for determining matrix multiplication ordering, that doesn't exist in EnzymeJAX but also like a 1 hr thing to add (atleast the greedy version).

@ChrisRackauckas
Copy link
Member

You added it 😅. I think it was to batch backwards passes, so instead of triangular solve on vectors it would batch it to triangular solve back a matrix.

@avik-pal
Copy link
Member

It was fa0cd34 (not me)

From discourse it seems to be the gemm ordering (I have a half finished PR for this in reactant).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants