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

PreconditionRelaxation: step w. relaxation #13285

Merged

Conversation

peterrum
Copy link
Member

Add specialization for preconditioners with:

void vmult(dst, src, relax);
void step(dst, src, relax);

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Looks good. It's time we get to use C++20 concepts :-)

I assume you're going to test this in a follow-up patch somewhere?


template <typename U>
static decltype(
std::declval<U const>().step(std::declval<VectorType &>(),
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with other place:

Suggested change
std::declval<U const>().step(std::declval<VectorType &>(),
std::declval<const U>().step(std::declval<VectorType &>(),


public:
static const bool value =
!std::is_same<bool, decltype(detect(std::declval<T>()))>::value;
Copy link
Member

Choose a reason for hiding this comment

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

Let's turn this around to make it read like "if this complicated type is 'bool'":

Suggested change
!std::is_same<bool, decltype(detect(std::declval<T>()))>::value;
!std::is_same<decltype(detect(std::declval<T>())),bool>::value;


template <typename U>
static decltype(
std::declval<U const>().Tstep(std::declval<VectorType &>(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::declval<U const>().Tstep(std::declval<VectorType &>(),
std::declval<const U>().Tstep(std::declval<VectorType &>(),


public:
static const bool value =
!std::is_same<bool, decltype(detect(std::declval<T>()))>::value;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!std::is_same<bool, decltype(detect(std::declval<T>()))>::value;
!std::is_same<decltype(detect(std::declval<T>())),bool>::value;

Comment on lines +580 to +584
static decltype(
std::declval<U const>().step(std::declval<VectorType &>(),
std::declval<const VectorType &>(),
std::declval<const double>()))
detect(const U &);
Copy link
Member

Choose a reason for hiding this comment

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

What do you do if the function, for whatever reason, returns bool? I suspect that's not very likely, but a common trick is to make the other function return an object of type X where X is a locally declared struct type. No member function of an external class can reasonably return such an X.

@masterleinad
Copy link
Member

Looks good. It's time we get to use C++20 concepts :-)

We could just roll our own is_detected for now.

@bangerth
Copy link
Member

I was thinking of requires clauses and proper concepts.

How does is_detected work?

@peterrum
Copy link
Member Author

@bangerth I used the following code as template:

// similar to type traits in FEEvaluation, below we add type-traits
// to distinguish between vectors that provide different interface for
// operations like update_ghost_values(), compress(), etc.
// see internal::has_local_element in fe_evaluation.h that documents
// how those type traits work.
// a helper type-trait that leverage SFINAE to figure out if type T has
// void T::update_ghost_values_start(const uint) const
template <typename T>
struct has_update_ghost_values_start
{
private:
static bool
detect(...);
template <typename U>
static decltype(std::declval<U const>().update_ghost_values_start(0))
detect(const U &);
public:
static const bool value =
!std::is_same<bool, decltype(detect(std::declval<T>()))>::value;
};
// We need to have a separate declaration for static const members
template <typename T>
const bool has_update_ghost_values_start<T>::value;

I don't know all details SFINAE. I don't know why these lines were written like this. Maybe we could proceed with this PR like it is and postpone a possible clean up to later: there are some function in matrix_free/type_traits.h and in lac/precondition.h like this.

@peterrum
Copy link
Member Author

I assume you're going to test this in a follow-up patch somewhere?

Yes.

@bangerth
Copy link
Member

Yes, if there is existing art that does it the same way, let's stick with it!

@bangerth bangerth merged commit ea6e238 into dealii:master Jan 24, 2022
@masterleinad
Copy link
Member

I was thinking of requires clauses and proper concepts.

How does is_detected work?

It pretty much does exactly what we are doing here but standardized (and shorter), see https://en.cppreference.com/w/cpp/experimental/is_detected. It might never hit the standard since there are concepts in C++20. Since it will take a good amount of time before requiring C++20 using a hand-rolled is_detected might be a good cleanup in the meantime.

@bangerth
Copy link
Member

I would accept a patch based on this.

@peterrum peterrum changed the title PreconditionRelaxation: vmult/step w. relaxation PreconditionRelaxation: step w. relaxation Jan 29, 2022
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

3 participants