Sparsity pattern insert #3338
Sparsity pattern insert #3338
Conversation
schnellerhase
force-pushed
the
sparsity_pattern_insert
branch
from
8f4c8e2 to
c1b5746
Compare
| @@ -140,14 +140,29 @@ SparsityPattern::SparsityPattern( | |||
| } | |||
| } | |||
| //----------------------------------------------------------------------------- | |||
| void SparsityPattern::insert(std::int32_t row, std::int32_t col) | |||
There was a problem hiding this comment.
Isn't this covered by passing rows and cols of length 1?
There was a problem hiding this comment.
It it, but the interface of the caller becomes cleaner. Instead of needing to call
sp.insert(std::vector<std::int32_t>{row}, std::vector<std::int32_t>{col});
we can now simply call
sp.insert(row, col);
same holds for the python interface with numpy arrays.
Also, in combination with the span based version it makes clearer how the sparsity pattern is to be used, i.e. loop over entries and dense matrices and do not pre-compute the entries and then pass it in bulk. Sparsity pattern already performs the caching and associated memory management.
There was a problem hiding this comment.
Well, it's clear from the improved docstring. I don't see the point of this new method, the clarity of overloading functions is debatable.
There was a problem hiding this comment.
To add, the interface is vectorised from the Python end to encourage users to pass in multiple entries, avoiding the overhead of the function call.
There was a problem hiding this comment.
How/where is the interface vectorized? I can not find that.
There was a problem hiding this comment.
@chrisrichardson mentioned the other day, we might instead of the integer version consider to create a non owning const std::vector<std::int32_t>& version, this should allow for the interface to be insert({row}, {col}), i.e. it allows for conversion from an initializer list.
cpp/dolfinx/la/SparsityPattern.h
Outdated
| /// This routine inserts non-zero locations at the outer product of rows and | ||
| /// cols to the sparsity pattern, i.e. adds the matrix entries at | ||
| /// A[row[i], col[j]] for all i, j | ||
| /// Enabling easy setting of dense sub-blocks, for example |
There was a problem hiding this comment.
Not sure about the word dense. What if I pass e.g. 1, 3, 4 1, 3, 4.
There was a problem hiding this comment.
The introduced entries then form a dense 3x3 entries again, not locally dense in the sparse matrix, but a dense matrix worth of entries - not possible to only specify a sub set of these with this function.
|
Same interface present for |
|
A related problem of span based interface is present in the global to local mappings of the |
* Add single index insert to sparsity pattern.
* Remove some superfluous {}
* export new insert function
* Make use of new insert functionality to simplify code
* Extend insert doc to make clear how the block insertion works
* Fix doc
* to -> into
* Remove duplicate comment
* Remove dense matrix example
* {} style adapted
* Missed some
* Update doctring
---------
Co-authored-by: Chris Richardson <[email protected]>
Introduces a new sparsity pattern insert functionality for a single entry - which is the method of choice used in multiple locations of the code base.
Additionally the documentation of the
insert(span<>, span<>)call on a sparsity pattern is extended to make clear how the inputs work together, and now hints that the interpretation of a pairwise operation is not correct to the caller.