Skip to content
This repository has been archived by the owner on Jul 19, 2023. It is now read-only.

Concretization cleanup #183

Merged
merged 1 commit into from
Sep 22, 2019
Merged

Concretization cleanup #183

merged 1 commit into from
Sep 22, 2019

Conversation

jagot
Copy link
Contributor

@jagot jagot commented Sep 22, 2019

Before starting to look at #182, I refactored the concretization, since so much was in common between the different array types.

Impacts #173.

stencil_pivot = div(stencil_length,2)
end
SparseArrays.SparseMatrixCSC(A::DerivativeOperator{T}, N::Int=A.len) where T =
copyto!(spzeros(T, N, N+2), A, N)
Copy link
Member

Choose a reason for hiding this comment

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

This would be really slow?

Copy link
Contributor Author

@jagot jagot Sep 22, 2019

Choose a reason for hiding this comment

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

Why would it? I overloaded Base.copyto! at the top of the file, the code of which is exactly the same as was in all three versions for different target matrices.

I might add we've used this pattern to great effect in ContinuumArrays.jl et al.

Copy link
Member

Choose a reason for hiding this comment

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

Adding to a sparse matrix using indexing is really slow: the indices should all be declared up front.

Copy link
Member

Choose a reason for hiding this comment

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

oh I see, ehh, it's not any worse than before, but it's still bad. If no one wants to actually do the (I,J,V), then it's best to probably just build the banded matrix and then convert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :) I was anyway interested in the banded versions, but could not keep my hands off refactoring this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, 99% of the time people should just call sparse and get the banded version.

@ChrisRackauckas ChrisRackauckas merged commit 39a3e99 into SciML:master Sep 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants