Skip to content
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

TpetraWrappers::Vector: Don't use rcp pointers internally #16549

Closed

Conversation

masterleinad
Copy link
Member

Alternative to #16645, addressing #16545 (comment).
@kinnewig It seems we can do without using rcp pointers in the TpetraWrappers::Vector implementation and don't need them in the interface either simplifying the implementation.

Comment on lines 65 to 66
, vector(V.vector, Teuchos::Copy)
, nonlocal_vector(V.nonlocal_vector, Teuchos::Copy)
Copy link
Member Author

Choose a reason for hiding this comment

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

The places fixing the CI are here...

{
*vector = *V.vector;
vector = VectorType(V.vector, Teuchos::Copy);
Copy link
Member Author

Choose a reason for hiding this comment

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

... and here.

Copy link
Member

@tamiko tamiko left a comment

Choose a reason for hiding this comment

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

Nice cleanup.

@kronbichler
Copy link
Member

I think this looks good. However, I have seen several application codes where Teuchos::RCP objects were used rather than the plain objects. The objective is typically to transfer ownership of the vector object between different functions (think of a programming model that avoids the subscription model we have with our dealii::SmartPointer and instead keep the object alive as long as it is used). From @masterleinad's comment in the other PR I learn that the vector itself is a pointer-like object in the sense that it does reference counting on its own, so such a model might no longer be necessary, as one might be able to pass the objects around. @kinnewig have you seen one model or the other used in the Trilinos examples you looked at (while implementing these interfaces), and would you agree with this change here in terms of what you have learned?

@jpthiele
Copy link
Contributor

jpthiele commented Jan 27, 2024

I think this looks good. However, I have seen several application codes where Teuchos::RCP objects were used rather than the plain objects. The objective is typically to transfer ownership of the vector object between different functions (think of a programming model that avoids the subscription model we have with our dealii::SmartPointer and instead keep the object alive as long as it is used). From @masterleinad's comment in the other PR I learn that the vector itself is a pointer-like object in the sense that it does reference counting on its own, so such a model might no longer be necessary, as one might be able to pass the objects around. @kinnewig have you seen one model or the other used in the Trilinos examples you looked at (while implementing these interfaces), and would you agree with this change here in terms of what you have learned?

You are correct. All of Belos (iterative solvers) builds upon the Belos::LinearProblem, which expects an RCP to the matrix and vector during construction. Amesos expects an RCP too with the alternative of a raw pointer because it mostly calls external packages like MUMPS which want a raw pointer anyways.

And if you look a the TPetra tutorials everything is constructed as RCPs as well.
And I know from talking with Trilinos developers that your life is easiest if everything is an RCP,
that is somewhat of a design principle to ensure objects are still valid during runtime
and as you said a similar safety net to the SmartPointer or std::shared_ptr (which I think was inspired by RCP).

I have tried using EPetra with non wrapped XPetra solvers and it was a hassle from a users perspective to
do everything correctly with the RCPs going from the raw unique_ptr there.
Therefore @kinnewig and I decided to internally use RCPs because it doesn't matter for the average user who stays in the Wrapper classes but makes extending those and going to 'raw' Trilinos a lot easier.

@kinnewig
Copy link
Contributor

I think this looks good. However, I have seen several application codes where Teuchos::RCP objects were used rather than the plain objects. The objective is typically to transfer ownership of the vector object between different functions (think of a programming model that avoids the subscription model we have with our dealii::SmartPointer and instead keep the object alive as long as it is used). From @masterleinad's comment in the other PR I learn that the vector itself is a pointer-like object in the sense that it does reference counting on its own, so such a model might no longer be necessary, as one might be able to pass the objects around. @kinnewig have you seen one model or the other used in the Trilinos examples you looked at (while implementing these interfaces), and would you agree with this change here in terms of what you have learned?

I have never seen an example where the raw Tpetra::Vector objects are used inside of Trilinos. As @jpthiele mentioned, the tutorials uses always Teuchos::RCP.

Part of the reason why I started to work on the Tpetra-Interface is because I want to use FROSch as a solver inside of deal.II. I already have a preliminary FROSch interface, and inside that interface, I need the RCP objects since FROSch expects RCPs instead of the raw vectors/matrices.
I also worked on FROSch itself. And FROSch uses RCP internally as well.

As mentioned by @jpthiele, also other classes, e.g. the Belos::LinearOperator, expects RCPs instead of raw vectors/matrices.

Since the goal of the Trilinos wrapper should be to make it as simple as possible for users to access Trilinos packages through deal.II, I would strongly suggest that we stick with RCPs.

*/
Teuchos::RCP<
const Tpetra::Vector<Number, int, types::signed_global_dof_index>>
trilinos_rcp() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not remove this function.

I already implemented a preliminary solver for Tpetra, here and that makes use of this function.
The solver would become way more complicated without this function.

@masterleinad
Copy link
Member Author

Note that we can construct a non-owning rcp, see https://docs.trilinos.org/dev/packages/teuchos/doc/html/classTeuchos_1_1RCP.html#aca5aaa0a0ee98f43a178e28ca5ade6b1, to satisfy interfaces that require a rcp type.

@jpthiele
Copy link
Contributor

Note that we can construct a non-owning rcp, see https://docs.trilinos.org/dev/packages/teuchos/doc/html/classTeuchos_1_1RCP.html#aca5aaa0a0ee98f43a178e28ca5ade6b1, to satisfy interfaces that require a rcp type.

Yes sure, that is what is done all over the place in the Epetra Wrappers for solvers and preconditioners.
And it results in a huge amount of avoidable boilerplate code that is hard to understand to a novice
and error prone in porting to a new solver.

Having an RCP means every meaningful task we do with a linear equation system after assembly can be done directly
with classes in the Tpetra stack. Again, making it easier for anyone wanting to use deal.II to assemble and non-wrapped solvers or parameters with Trilinos directly.

And looking at the PR almost all changes are from -> to . so in my mind no large saving compared to the inconvenience down the road.

So my main question is:
What do we actually gain from removing an RCP internally? Especially with all the downstream losses in solvers?

@masterleinad
Copy link
Member Author

What do we actually gain from removing an RCP internally? Especially with all the downstream losses in solvers?

It adds an unnecessary level of indirection. My comment above meant to say that we can still return an unmanaged rcp object so the interface stays the same. AFAICT, we never want shared ownership for these objects anyway from a deal.II point of view so using an unmanaged rcp should be fine.

And looking at the PR almost all changes are from -> to . so in my mind no large saving compared to the inconvenience down the road.

These changes are possible independent of the discussion here. Let me pull that out of this pull request.

@bangerth
Copy link
Member

Please update now that #16566 is merged.

I have no opinion about the matter at hand. If the rest of Trilinos is using RCPs, then I can see the reason to use RCPs here as well. I think that in practice, a single pointer indirection is likely not going to be too much of a problem. I do get that from a conceptual perspective, using the object outright looks nicer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants