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

Make Tpetra wrapper size_type definitions uniform. #16671

Open
bangerth opened this issue Feb 19, 2024 · 4 comments
Open

Make Tpetra wrapper size_type definitions uniform. #16671

bangerth opened this issue Feb 19, 2024 · 4 comments
Milestone

Comments

@bangerth
Copy link
Member

As mentioned in #16670, different parts of the Tpetra wrappers define their own size_type types, and some of these are signed:

include/deal.II/lac/trilinos_tpetra_block_sparse_matrix.h:      using size_type       = typename BaseClass::size_type;
include/deal.II/lac/trilinos_tpetra_block_vector.h:      using size_type       = typename BaseClass::size_type;
include/deal.II/lac/trilinos_tpetra_solver_direct.h:      using size_type = dealii::types::signed_global_dof_index;   ******
include/deal.II/lac/trilinos_tpetra_sparse_matrix.h:      using size_type = dealii::types::global_dof_index;
include/deal.II/lac/trilinos_tpetra_sparse_matrix.templates.h:      using size_type = dealii::types::signed_global_dof_index;  ******
include/deal.II/lac/trilinos_tpetra_sparsity_pattern.h:        using size_type = dealii::types::signed_global_dof_index;  ******
include/deal.II/lac/trilinos_tpetra_sparsity_pattern.h:        using size_type = size_t;  ******
include/deal.II/lac/trilinos_tpetra_sparsity_pattern.h:      using size_type = dealii::types::signed_global_dof_index;  ******
include/deal.II/lac/trilinos_tpetra_vector.h:      using size_type = dealii::types::global_dof_index;
include/deal.II/lac/trilinos_tpetra_vector.h:      using size_type  = types::global_dof_index;

It is perhaps not the best design to have these types be all over the place. We might want to make this more uniform before the next release.

@jpthiele @kinnewig

@bangerth bangerth added this to the Release 9.6 milestone Feb 19, 2024
@jpthiele
Copy link
Contributor

I agree that we should make this consistent.
The global ordinate in Tpetra is however signed as some negative values are used as error codes.
So it should be the same signed_global_dof_index across the board.

@bangerth
Copy link
Member Author

That's ok for the internal interfaces, but I think for the external uses, we want to use what deal.II has always used: unsigned integers for quantities that can only be zero or positive.

@jpthiele
Copy link
Contributor

Ah okay yes, so to return m() for example it should be unsigned.
Which is fine since the nonegative signed integers are a subset of the unsigned integers for each specific bitlength.
But we should possibly add an Assert that m internally is nonnegative and throw a TrilinosException otherwise?

@bangerth
Copy link
Member Author

Perhaps. I'm not overly concerned about that in the current case that it could happen.

In general, if Trilinos returns errors as negative numbers, we need to capture the error and pass it on via the usual deal.II mechanisms (via Assert or AssertThrow). If Trilinos returns error via exceptions, then I think we can just do an implicit conversion between the integer types and assume that Trilinos will just never return a negative value. That's because returning a negative value would be a bug in Trilinos (unless they specifically say that that's how they deal with errors) and we do not have to guard every call to Trilinos for possible bugs.

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

No branches or pull requests

2 participants