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
Implement a proper sparsity pattern #888
base: master
Are you sure you want to change the base?
Conversation
Supersedes #596 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this one Fredrik! It looks really awesome. :) I roughly went over the implementation and have some things which I like to discuss.
Since I could not annotate the files: I think you missed updating the the dg example and the integration test.
|
||
```julia | ||
K = create_matrix(dh) | ||
K = create_symmetric_sparsity_pattern(dh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, missed this. Actually I removed symmetric support on the pattern level, but this is one point to discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolved this since this looks inconsistent here, shouldn't it at least be create_symmetric_matrix(dh)
, or removed if not used anymore?
# 1. Create empty pattern | ||
sparsity_pattern = SparsityPattern(ndofs(dh), ndofs(dh)) | ||
# 2. Add entries | ||
create_sparsity_pattern!(sparsity_pattern, dh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should mention around here how to manually add entries (via Ferrite.add_entry!
).
test/test_dofs.jl
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add test coverage manually adjusting the sparsity pattern via add_entry!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I haven't really written tests for the new stuff, focused on getting old tests to pass.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #888 +/- ##
==========================================
- Coverage 93.86% 93.29% -0.57%
==========================================
Files 36 38 +2
Lines 5310 5520 +210
==========================================
+ Hits 4984 5150 +166
- Misses 326 370 +44 ☔ View full report in Codecov by Sentry. |
cross_coupling::Union{AbstractMatrix{Bool}, Nothing}, | ||
topology, | ||
) | ||
# 1. Add all connections between dofs for every cell while filtering based |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe factor this out similar to cross_element_coupling!
so it is possible to call it from user code building custom pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is basically just or adding the dofs for the cells so why not use this directly?
|
||
Used internally for sparsity patterns with cross-element coupling. | ||
TODO: Expose to users? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would lean towards answering this with a "yes" to give users more fine grained control (and modularity) over the sparsity pattern. And maybe we should discuss the name again in this case.
Before cleaning up and finishing this PR there are some things I would like input on:
|
I think this is a good idea not only for this reason. New users might not understand the difference between sparsity pattern and the actual sparse matrix and might try to pass the pattern to the assembler anyway. Alternatively we can also not error in this case and just assemble into a standard matrix format, which can be queried by
I think this is a great convenience for the large portion of our user base.
I think I am not understanding the issue here. If we want to have a very specific matrix, then we should be specific here. Not sure if |
Nice work @fredrikekre!
|
Perhaps a stupid question, but would it not make sense to support |
The issue is that it looks ugly to have something like A = create_matrix(BlockMatrix{Float64, Matrix{SparseMatrixCSR{1, Float64, Int64}}}, sparsity_pattern) But with aliases we could have e.g. A = create_matrix(MatrixTypes.BlockMatrix{MatrixTypes.CSRMatrix{Float64, Int}}, sparsity_pattern) A method for that would be defined in an extension still. The point is that type parameters isn't always "user-friendly" so with MatrixTypes we could normalize it a bit. But we can do this later as Knut points out.
The matrix type doesn't necessarily map directly to one pattern type. And if you want a specific pattern type, why not call that constructor directly ( As it is right now, |
If |
BlockSparsityPattern is in Ferrite now, but even with the hack you could still call the constructor (but get an error telling you to load the needed package which you would have to do anyway in order to pass the matrix type?). |
Note that the assembler itself is still tightly coupled to SparseMatrixCSC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice description (and work on this PR, looking forward to the benchmarks 🚀)
Added some suggestions also to make it faster to get to the main points at the end of the docs!
|
||
## Sparsity pattern | ||
|
||
The sparse structure of the linear system depends on many factors such as e.g. the weak |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn here since the derivative is not always the definition of the matrix (but implicitly assumed in the text below for e.g. reference to Poisson's equation). Yet, I find it logical introduce the coupling via the derivative, e.g.
In general, a finite element problem requires solving an equation system
to find
or for linear problems, a system matrix is constructed as
Each entry,
With the current text, it is for example not obvious that i
and j
couple").
(Not sure if it makes sense to discuss cases when using alternative techniques for defining the stiffness matrix via quasi-newton methods here, perhaps just mention that when discussing the coupling
keyword?)
The sparsity, i.e. the ratio of zero-entries to the total number of entries, is often *very* | ||
high and taking advantage of this results in huge savings in terms of memory. For example, | ||
in a problem with ``10^6`` DoFs there will be a matrix of size ``10^6 \times 10^6``. If all | ||
``10^{12}`` entries of this matrix had to be stored (0% sparsity) as double precision | ||
(`Float64`, 8 bytes) it would require 8 TB of memory. If instead the sparsity is 99.9973% | ||
(which is the case when solving the heat equation on a three dimensional hypercube with | ||
linear Lagrange interpolation) this would be reduced to 216 MB. | ||
|
||
|
||
### Sparsity pattern example | ||
|
||
To give an example, in this one-dimensional heat problem (see the [Heat | ||
equation](../tutorials/heat_equation.md) tutorial for the weak form) we have 4 nodes with 3 | ||
elements in between. For simplicitly DoF numbers and node numbers are the same but this is | ||
not true in general since nodes and DoFs can be numbered independently (and in fact are | ||
numbered independently in Ferrite). | ||
|
||
``` | ||
1 ----- 2 ----- 3 ----- 4 | ||
``` | ||
|
||
Assuming we use linear Lagrange interpolation (the "hat functions") this will give the | ||
following connections according to the weak form: | ||
- Trial function 1 couples with test functions 1 and 2 (entries `(1, 1)` and `(1, 2)` | ||
included in the sparsity pattern) | ||
- Trial function 2 couples with test functions 1, 2, and 3 (entries `(2, 1)`, `(2, 2)`, and | ||
`(2, 3)` included in the sparsity pattern) | ||
- Trial function 3 couples with test functions 2, 3, and 4 (entries `(3, 2)`, `(3, 3)`, and | ||
`(3, 4)` included in the sparsity pattern) | ||
- Trial function 4 couples with test functions 3 and 4 (entries `(4, 3)` and `(4, 4)` | ||
included in the sparsity pattern) | ||
|
||
The resulting sparsity pattern would look like this: | ||
|
||
``` | ||
4×4 SparseArrays.SparseMatrixCSC{Float64, Int64} with 10 stored entries: | ||
0.0 0.0 ⋅ ⋅ | ||
0.0 0.0 0.0 ⋅ | ||
⋅ 0.0 0.0 0.0 | ||
⋅ ⋅ 0.0 0.0 | ||
``` | ||
|
||
Moreover, if the problem is solved with periodic boundary conditions, for example by constraining the | ||
value on the right side to the value on the left side, there will be additional couplings. | ||
In the example above, this means that DoF 4 should be equal to DoF 1. Since DoF 4 is | ||
constrained it has to be eliminated from the system. Existing entries that include DoF 4 are | ||
`(3, 4)`, `(4, 3)`, and `(4, 4)`. Given the simple constraint in this case we can simply | ||
replace DoF 4 with DoF 1 in these entries and we end up with entries `(3, 1)`, `(1, 3)`, and | ||
`(1, 1)`. This results in two new entries: `(3, 1)` and `(1, 3)` (entry `(1, 1)` is already | ||
included). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is a very nice introductory text, I think it hides the important things below.
Perhaps add at least the example in a collapsible block?
(CSR)*](https://en.wikipedia.org/wiki/Sparse_matrix#Compressed_sparse_row_(CSR,_CRS_or_Yale_format)) | ||
format or the [*compressed sparse column | ||
(CSC)*](https://en.wikipedia.org/wiki/Sparse_matrix#Compressed_sparse_column_(CSC_or_CCS)) | ||
format, which is the default sparse matrix type implemented in the SparseArrays standard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format, which is the default sparse matrix type implemented in the SparseArrays standard | |
format, where the latter is the default sparse matrix type implemented in the SparseArrays standard |
the matrix directly. For example, most examples in this documentation don't deal with | ||
the sparsity pattern explicitly. | ||
|
||
### Costum sparsity patterns construction in Ferrite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Costum sparsity patterns construction in Ferrite | |
### Custom sparsity patterns construction in Ferrite |
[`init_sparsity_pattern(dh)`](@ref) function or by using a constructor directly. | ||
`init_sparsity_pattern` will return a default pattern type that is compatible with the | ||
DofHandler. In some cases you might require another type of pattern (for example a | ||
blocked pattern) and in that case you can use the constructor directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocked pattern) and in that case you can use the constructor directly. | |
blocked pattern, see [Blocked sparsity pattern](@ref)) and in that case you can use the constructor directly. |
2. *Add entries to the pattern.* There are a number of functions that add entries to the | ||
pattern: | ||
- [`create_sparsity_pattern!`](@ref) adds entries for all couplings between the DoFs | ||
within each element. These entries correspond to assembling the standard element | ||
matrix and is thus almost always required. | ||
- [`cross_element_coupling!`](@ref) adds entries for couplings between the DoFs in | ||
neighboring elements. These entries are required when integrating along internal | ||
interfaces between elements (e.g. for discontinuous Galerkin methods). | ||
- [`Ferrite.add_entry!`](@ref) adds a single entry to the pattern. This can be used if | ||
you need to add custom entries that are not covered by the other functions. | ||
|
||
3. *Condense the pattern.* This is done with the [`condense_sparsity_pattern!`](@ref) | ||
function and adds entries required from constraints and boundary conditions in the | ||
ConstraintHandler. Note that condensing the pattern *must* be done as the last operation | ||
on the pattern, e.g. after adding all other entries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on Slack...
2. *Add entries to the pattern.* There are a number of functions that add entries to the | |
pattern: | |
- [`create_sparsity_pattern!`](@ref) adds entries for all couplings between the DoFs | |
within each element. These entries correspond to assembling the standard element | |
matrix and is thus almost always required. | |
- [`cross_element_coupling!`](@ref) adds entries for couplings between the DoFs in | |
neighboring elements. These entries are required when integrating along internal | |
interfaces between elements (e.g. for discontinuous Galerkin methods). | |
- [`Ferrite.add_entry!`](@ref) adds a single entry to the pattern. This can be used if | |
you need to add custom entries that are not covered by the other functions. | |
3. *Condense the pattern.* This is done with the [`condense_sparsity_pattern!`](@ref) | |
function and adds entries required from constraints and boundary conditions in the | |
ConstraintHandler. Note that condensing the pattern *must* be done as the last operation | |
on the pattern, e.g. after adding all other entries. | |
2. **Add entries to the pattern:** This can conveniently be done by using [`create_sparsity_pattern!`](@ref). If a finer-grained control is required, there are a number of functions that add entries to the | |
pattern: | |
- [`cell_couplings!`](@ref) adds entries for all couplings between the DoFs | |
within each element. These entries correspond to assembling the standard element | |
matrix and is thus almost always required. | |
- [`cross_element_coupling!`](@ref) adds entries for couplings between the DoFs in | |
neighboring elements. These entries are required when integrating along internal | |
interfaces between elements (e.g. for discontinuous Galerkin methods). | |
- [`Ferrite.add_entry!`](@ref) adds a single entry to the pattern. This can be used if | |
you need to add custom entries that are not covered by the other functions. | |
- [`condense_sparsity_pattern!`](@ref) adds entries required from constraints and boundary conditions in the | |
ConstraintHandler. Note that condensing the pattern *must* be done as the last operation | |
on the pattern, e.g. after adding all other entries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the extra comments that I think constraint_couplings!
is a better name than condense_sparsity_pattern!
.
Side-note: Sounds like having to call condense_sparsity_pattern!
last can easily lead to user bugs. Would it make sense to add a condensed::Ref{Bool}
(or ScalarWrapper
) in SparsityPattern
to allow the other methods error if trying to add entries after condensation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you dont call the condense function last, I think you will at least get an error in the assembly-routine.
I think the name change make sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the extra comments that I think
constraint_couplings!
is a better name thancondense_sparsity_pattern!
.
The process is called condensation, right? So why should we use a different name for this?
Side-note: Sounds like having to call
condense_sparsity_pattern!
last can easily lead to user bugs. Would it make sense to add acondensed::Ref{Bool}
(orScalarWrapper
) inSparsityPattern
to allow the other methods error if trying to add entries after condensation?
I think this is a really good idea.
!!! note "Swiss army knife" | ||
The [`create_sparsity_pattern!`](@ref) function is something of a swiss army knife since | ||
it can do more than just adding the basic element couplings as described above. For | ||
example, when passed the correct arguments, it can also do the job of | ||
`cross_element_coupling!` and `condense_sparsity_pattern!`. Refer to the API reference | ||
for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be removed if adopting my suggestion with cell_couplings!
above
## Creating the sparsity pattern | ||
|
||
Given a `DofHandler` we can obtain the corresponding sparse matrix by using the | ||
[`create_sparsity_pattern`](@ref) function. This will setup a `SparseMatrixCSC` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[`create_sparsity_pattern`](@ref) function. This will setup a `SparseMatrixCSC` | |
[`create_matrix`](@ref) function. This will setup a `SparseMatrixCSC` |
?
a boolean value indicating if two DoFs should couple or not. The function should take two | ||
arguments, the first being the element index and the second being the DoF index within the | ||
element. The function should return `true` if the DoFs should couple and `false` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence doesn't make sense to me.
I would have expected that f(i, j)
should say if DoF i
couples to DoF j
? But here it sounds like the cellid
is expected, but perhaps I'm just confused with array element vs finite element?
Wasn't there an option to give the field coupling via a Matrix{Bool}
before?
Side-note: Probably giving uncoupled::Vector{Pair{Symbol,Symbol}}
would be more user-friendly to remove couplings instead, but can be done later...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is Copilot hallucinations I forgot to remove haha
|
||
```julia | ||
K = create_matrix(dh) | ||
K = create_symmetric_sparsity_pattern(dh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolved this since this looks inconsistent here, shouldn't it at least be create_symmetric_matrix(dh)
, or removed if not used anymore?
I have another consideration which came to my mind. Is |
To me, it makes sense to have this function to do the "right thing" (such that it will work) given the inputs. I thought that if you give it a dofhandler with DG-interpolations it will call For advanced use-cases (for example if you have additional info that can increase the sparsity), I think the separation with the extra functions are really nice, but I don't think we need to add extra convenience for many different cases (at least not yet). |
Yea, right now |
This only happens if you also pass the topology. You cannot differentiate between DG and "normal" P0 on interpolation level, as the coupling is a property of the weak form and not of the interpolation. |
This PR introduces
struct SparsityPattern
for a way to work with the pattern directly instead of producing a matrix directly.Some advantages of this are:
ndofs(dh) x ndofs(dh)
so it is easy to insert new dofsSparseMatrixCSC
(e.g. how element connections and constraint condensation were implemented). With this patch everything is handled on theSparsityPattern
level independent of what type of matrix will be instantiated in the end. All that is needed to support a new matrix type is now to write aMatrixType(::SparsityPattern)
constructor.