-
Notifications
You must be signed in to change notification settings - Fork 707
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
Introduce SUNDIALS N_Vector module #11395
Conversation
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 highlighted some comments and questions
*/ | ||
template <typename VectorType> | ||
const VectorType * | ||
unwrap_nvector_const(N_Vector v); |
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.
Not sure if the calls should explicitly mention the const-ness. Alternative would be to enable something like unwrap_nvector<const VectorType>(...)
but this feels unintuitive to me.
AssertDimension(x_dealii->size(), z_dealii->size()); | ||
AssertDimension(x_dealii->size(), y_dealii->size()); | ||
|
||
std::transform(x_dealii->begin(), |
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 is an example of a vector operation that is not easily translated to the dealii vector interface. I went for this naive implementation for now to have a working version in place.
// v->ops->nvwrmsnormmask = undef; | ||
// v->ops->nvmin = undef; | ||
// v->ops->nvwl2norm = undef; | ||
// v->ops->nvl1norm = undef; | ||
// v->ops->nvcompare = undef; | ||
// v->ops->nvinvtest = undef; | ||
// v->ops->nvconstrmask = undef; | ||
// v->ops->nvminquotient = undef; |
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.
For reference, I put some further operations that could be implemented here. In a quick test with the ARKode tests they didn't seem to be necessary.
// --------------------------------------------------------------------- | ||
|
||
// serial Vector and BlockVector | ||
for (S : REAL_SCALARS; V : DEAL_II_VEC_TEMPLATES) |
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 went for a suite of vectors that I thought are representative and use myself. Extension to more vectors should be straight-forward.
tests/sundials/n_vector.cc
Outdated
NVectorView<VectorType> n_vector(vector); | ||
auto id = N_VGetVectorID(n_vector); |
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.
These lines demonstrate the usage of NVectorView
. After creating the view, it can be passed to a SUNDIALS function where it implicitly converts to N_Vector
. The view is cleaned up when going out of scope.
@kronbichler Can you have a look? This PR is all about dealii vectors, their operations and memory management. Virtually no SUNDIALS knowledge required;) |
ef3cc23
to
c527760
Compare
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.
Looks very mature already 🥇 It took some time to understand why so many indirections are needed. One creates a view on deal.II vector NVectorView
, which internally creates a NVectorContent
. The latter classes can have two states: either it only stores only a pointer to the original vector or it "stores" the actual vector (created by the clone function - or does it?).
Maybe you could provide a small code snippet which shows how unwrap_nvector()
, unwrap_nvector_const()
, and NVectorView
will be used in the ARKODE classes. That might help the next reviewer.
include/deal.II/sundials/n_vector.h
Outdated
* @note N_VDestroy() should not be called on the resulting N_Vector since | ||
* this would lead to a double delete. Let the destructor do this work. |
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.
Merge this into the paragraph before. Is there a check?
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.
No and I couldn't come up with an easy one. The problem is that destroy
also calls free
on the N_Vector.
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 you could provide a small code snippet which shows how unwrap_nvector(), unwrap_nvector_const(), and NVectorView will be used in the ARKODE classes. That might help the next reviewer.
@peterrum I hope the first three comments in this review help.
In addition, I highlighted a new function make_nvector_view
that could be used as a nicer(?) way to construct the view objects. However, it raises some other design questions for the NVectorView
auto vector = create_test_vector<VectorType>(); | ||
const auto const_vector = create_test_vector<VectorType>(); |
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.
Usage of the new functionality (1/3)
Let's say we have two deal.II vectors vector
and const_vector
.
auto n_vector = make_nvector_view(vector); | ||
auto const_n_vector = make_nvector_view(const_vector); |
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.
Usage of the new functionality (2/3)
Here, we create two view objects that can be passed to any SUNDIALS function that takes an N_Vector
. One would use this approach e.g. to pass an initial solution vector to SUNDIALS.
Assert((const_n_vector)->content != nullptr, NVectorTestError()); | ||
|
||
// unwrap non-const as non-const | ||
auto *vector_unwrapped = unwrap_nvector<VectorType>(n_vector); |
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.
Usage of the new functionality (3/3)
This is the oppposite operation: a N_Vector is unwrapped to extract the underlying deal.ii vector. One would use this appraoch e.g. to pass on a dst
vector from SUNDIALS to a deal.ii linear solver. Note that after the linear solve no copies or updates must be made since both n_vector
and vector_unwrapped
refer to the same memory.
*/ | ||
template <typename VectorType> | ||
NVectorView<VectorType> | ||
make_nvector_view(VectorType &vector); |
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 introduced this new function to utilize template parameter deduction.
* auto view = make_nvector_view(vector); | ||
* @endcode | ||
*/ | ||
NVectorView() = default; |
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.
See the doc string. I am not 100% sure if this is required. With gcc 7.3.1 it also worked without a default constructor.
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.
Note to myself: seems that this is not required. I will remove it.
/** | ||
* Constructor. Create view of @p vector. | ||
*/ | ||
NVectorView(VectorType &vector); |
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.
Could be made private if make_nvector_view is declared as friend. Currently, one could still use the constructor.
Correct, to put it differently: the class Only |
@peterrum @luca-heltai @kronbichler How should we proceed here? |
/rebuild |
I think we should merge this, and adjust later if necessary. With this, it should be much easier to interface with sundials, and I think it is a big step forward. I'm in favour of merging early, testing a bit the new interface, and then adjust later if necessary. |
Okay! I already have some adaptions to the ARKode integrator to use the new N_Vector module. Will follow up with it soon. |
This PR proposes a dealii wrapper for the SUNDIALS' N_Vector module as part of #11380. All of the functionality is in
internal
because the conversion between vectors is not of interest for a user of a SUNDIALS integrator. I tried to further separate the only relevant functions for an author of the ARKODE, IDA, etc. wrapper inton_vector.h
. The functions are:NVectorView
, a class to view aVectorType
as an N_Vector andunwrap_nvector()
andunwrap_nvector_const()
which are used to retrieve the internally stored dealii vector from an N_Vector.This PR doesn't yet make use of the new functionality in any time integrator. I will wait for #11278 to do so but I have already checked that it works (for ARKode at least). Also, there are tests for all the vector operations that are already supported.
Commit b3f1761 contains the relevant changes. The later commits are to (hopefully) fix the tests. I suggest to rebase when #11278 is merged. Note that the tests are not actually run here since they currently require SUNDIALS > 5.0.0. I can later backport functionality or, if support for older versions will be ended in the foreseeable future, we can keep the old copying mechanism for those versions.