Skip to content

Make SparseArrays a regular dependency of OrdinaryDiffEqCore.jl #3031

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.

Sign up for GitHub

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
JoshuaLampert wants to merge 2 commits into from
Closed

Make SparseArrays a regular dependency of OrdinaryDiffEqCore.jl #3031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b156c1
make SparseArrays a regular dependency of OrdinaryDiffEqCore.jl
JoshuaLampert Feb 2, 2026
84ee375
remove SparseArrays from targets
JoshuaLampert Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions lib/OrdinaryDiffEqCore/Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "OrdinaryDiffEqCore"
uuid = "bbf590c4-e513-4bbe-9b18-05decba2e5d8"
authors = ["ParamThakkar123 <paramthakkar864@gmail.com>"]
version = "3.4.0"
version = "3.4.1"

[deps]
SciMLOperators = "c0aeaf25-5076-4817-a8d5-81caf7dfa961"
Expand Down Expand Up @@ -37,6 +37,7 @@ DiffEqBase = "2b5f629d-d688-5b77-993f-72d75c75574e"
FillArrays = "1a297f60-69ca-5386-bcde-b61e274b549b"
Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e"
Reexport = "189a3867-3050-52da-a836-e630ba90ab69"
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"

[extras]
JET = "c3a54625-cd67-489e-a8e7-0a5a0ff4e31b"
Expand Down Expand Up @@ -98,12 +99,10 @@ Reexport = "1.2"
[weakdeps]
Mooncake = "da2b9cff-9c12-43a0-ae48-6db2b0edb7d6"
EnzymeCore = "f151be2c-9106-41f4-ab19-57ee4f262869"
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"

[targets]
test = ["DiffEqDevTools", "Random", "SafeTestsets", "SparseArrays", "Test", "Pkg"]
test = ["DiffEqDevTools", "Random", "SafeTestsets", "Test", "Pkg"]

[extensions]
OrdinaryDiffEqCoreEnzymeCoreExt = "EnzymeCore"
OrdinaryDiffEqCoreMooncakeExt = "Mooncake"
OrdinaryDiffEqCoreSparseArraysExt = "SparseArrays"
25 changes: 0 additions & 25 deletions lib/OrdinaryDiffEqCore/ext/OrdinaryDiffEqCoreSparseArraysExt.jl
View file
Open in desktop

This file was deleted.

2 changes: 2 additions & 0 deletions lib/OrdinaryDiffEqCore/src/OrdinaryDiffEqCore.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ using MuladdMacro: @muladd

using LinearAlgebra: opnorm, I, UniformScaling, diag, rank, isdiag

import SparseArrays

import PrecompileTools

import FillArrays: Trues, Falses
Expand Down
20 changes: 19 additions & 1 deletion lib/OrdinaryDiffEqCore/src/misc_utils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,27 @@ function get_differential_vars(f, u)
end

# Fallback for _isdiag - uses LinearAlgebra.isdiag which is O(n²)
# Sparse specialization is provided in OrdinaryDiffEqCoreSparseArraysExt
_isdiag(A::AbstractMatrix) = isdiag(A)

# Efficient O(nnz) isdiag check for sparse matrices.
# Standard isdiag is O(n²) which is prohibitively slow for large sparse matrices.
"""
_isdiag(A::SparseMatrixCSC)

Check if a sparse matrix is diagonal in O(nnz) time by traversing the CSC structure directly.
Returns `true` if all non-zero elements are on the diagonal.
"""
function _isdiag(A::SparseArrays.SparseMatrixCSC)
m, n = size(A)
m != n && return false
@inbounds for j in 1:n
for k in A.colptr[j]:(A.colptr[j + 1] - 1)
A.rowval[k] != j && return false
end
end
return true
end

Comment on lines +138 to +156
Copy link
Copy Markdown
Member

@ranocha ranocha Feb 2, 2026

Choose a reason for hiding this comment

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

Has this been upstreamed to SparseArrays?

Copy link
Copy Markdown
Member

@oscardssmith oscardssmith Feb 2, 2026

Choose a reason for hiding this comment

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

no, but it should be.

Copy link
Copy Markdown
Contributor Author

@JoshuaLampert JoshuaLampert Feb 2, 2026

Choose a reason for hiding this comment

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

Yes, I agree. So should this PR just revert #3011?

Copy link
Copy Markdown
Member

@oscardssmith oscardssmith Feb 2, 2026

Choose a reason for hiding this comment

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

no. We should open this upstream, but keep this optimization in the meantime.

Copy link
Copy Markdown
Contributor Author

@JoshuaLampert JoshuaLampert Feb 2, 2026

Choose a reason for hiding this comment

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

This is breaking CI in at least 5-10 our repos. This is pretty urgent for us.

Copy link
Copy Markdown
Member

@oscardssmith oscardssmith Feb 2, 2026

Choose a reason for hiding this comment

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

Isn't the breaking part that this used to be in an extension? Shouldn't bringing it into OrdinaryDiffEqCore fix your issue?

Copy link
Copy Markdown
Contributor Author

@JoshuaLampert JoshuaLampert Feb 2, 2026

Choose a reason for hiding this comment

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

Yes, if we can merge this PR as is that would also be fine as a hot fix. I thought you wanted to keep the status quo until this is upstreamed to SparseArrays.jl.

Copy link
Copy Markdown
Member

@oscardssmith oscardssmith Feb 2, 2026

Choose a reason for hiding this comment

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

sorry for the confusion. I meant keeping the status quo of implimenting _isdiag ourselves, but changing where it lives to Core.

Copy link
Copy Markdown
Contributor Author

@JoshuaLampert JoshuaLampert Feb 2, 2026

Choose a reason for hiding this comment

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

Yes, in other words: merging this PR because changing where _isdiag lives is exactly what this PR does. That's fine with me. Or do you mean something else than this PR currently does?

isnewton(::Any) = false

function _bool_to_ADType(::Val{true}, ::Val{CS}, _) where {CS}
Expand Down
1 change: 0 additions & 1 deletion lib/OrdinaryDiffEqCore/test/sparse_isdiag_tests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using Test
# Load SparseArrays first to trigger the extension
using SparseArrays
using OrdinaryDiffEqCore: _isdiag

Expand Down
Loading