Generalize CSC assembler #916
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #916 +/- ##
==========================================
- Coverage 93.65% 93.63% -0.03%
==========================================
Files 39 39
Lines 6003 6013 +10
==========================================
+ Hits 5622 5630 +8
- Misses 381 383 +2 ☔ View full report in Codecov by Sentry. |
termi-official
added
needs changelog
awaiting review
termi-official
removed
the
awaiting review
termi-official
added
the
awaiting review
|
Thanks for the review Knut! I would like to suggest that the removal of the symmetric API should go into a separate PR. |
There was a problem hiding this comment.
LGTM.
I like the name-changes, but perhaps there should be a @deprecate_binding or something @fredrikekre ?
I just think we are at a point now that we need to reduce annoyance to users where possible during the upgrade.
|
This should be ready again. |
| @@ -73,15 +73,15 @@ end; | |||
| # | |||
| # ScratchValues is a thread-local collection of data that each thread needs to own, | |||
| # since we need to be able to mutate the data in the threads independently | |||
| struct ScratchValues{T, CV <: CellValues, FV <: FacetValues, TT <: AbstractTensor, dim, Ti} | |||
| struct ScratchValues{T, CV <: CellValues, FV <: FacetValues, TT <: AbstractTensor, dim, AT <: Ferrite.AbstractSparseAssembler} | |||
There was a problem hiding this comment.
I'd suggest not "documenting" the abstract type for now, since there is no clear interface.
| struct ScratchValues{T, CV <: CellValues, FV <: FacetValues, TT <: AbstractTensor, dim, AT <: Ferrite.AbstractSparseAssembler} | |
| struct ScratchValues{T, CV <: CellValues, FV <: FacetValues, TT <: AbstractTensor, dim, AT} |
src/assembler.jl
Outdated
| end | ||
|
|
||
| finish_assemble(a::AbstractSparseCSCAssembler) = get_matrix(a) |
There was a problem hiding this comment.
Why is this needed? Not covered by tests and I think this shouldn't be encouranged since it doesn't "do" anything.
There was a problem hiding this comment.
I need this interface for making virtually any assembly format efficient that is not vanilla CSR or CSC. E.g. EA or ParCSR, which need a synchronization phase after all data is there (instead of syncing potentially each element individually).
There was a problem hiding this comment.
It is not clear what this should return if you have a matrix + a vector to assemble for example?
I would find a two step version more logical, I.e. finish_assemble!(a); K = get_matrix(a). (Not sure what the finish function should be called). Perhaps materialize! (or close!)
| @@ -33,24 +40,28 @@ as described in the [manual](@ref man-assembly). | |||
| the same pattern. See the [manual section](@ref man-assembly) on assembly. | |||
| """ | |||
| function start_assemble(N::Int=0) | |||
| return Assembler(N) | |||
| return start_assemble(Float64, N) | |||
There was a problem hiding this comment.
| return start_assemble(Float64, N) | |
| return COOAssembler(N) |
fredrikekre
force-pushed
the
do/more-generic-csc-assembler
branch
from
1e0e05c to
2b22a3e
Compare
* Generalize to use Abstract[Cell/Facet]Values in codebase * Apply renaming following Ferrite-FEM/Ferrite.jl#916 * Change default states to Vector{Nothing} instead of Nothing
These changes should be sufficient to allow assembly into CSC matrix types other than SparseMatrixCSC and the Symmtric counterpart.
Also a step towards shaping the interface for #628 , since resize is not allowed on the GPU.
TODOs