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

Implement StridedArrayView #15972

Merged
merged 2 commits into from Sep 17, 2023
Merged

Conversation

bergbauer
Copy link
Contributor

Needed for #15971

* This is particularly useful when you want to access only one lane of a
* VectorizedArray.
*/
template <typename ElementType, int stride = 1>
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
template <typename ElementType, int stride = 1>
template <typename ElementType, std::size_t stride = 1>

Comment on lines 76 to 80
StridedArrayView(value_type *starting_element, const std::size_t n_elements)
{
this->starting_element = starting_element;
this->n_elements = n_elements;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the definition out of line of the class declaration?

Also, please use the initializer list feature:

Suggested change
StridedArrayView(value_type *starting_element, const std::size_t n_elements)
{
this->starting_element = starting_element;
this->n_elements = n_elements;
}
StridedArrayView(value_type *starting_element, const std::size_t n_elements)
: starting_element (starting_element)
, n_elements (n_elements)
{}

Comment on lines 86 to 90
std::size_t
size() const
{
return this->n_elements;
}
Copy link
Member

Choose a reason for hiding this comment

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

Move the definition out of line. Same for the functions below.

@bergbauer
Copy link
Contributor Author

@bangerth Updated!

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

I think this separation makes sense. I will not approve because I was involved in discussions. One thing I will mention is that we discussed that it is good to provide the pointer iterators begin and end only for the actual ArrayView class, because for a strided class one would need non-standard iterators, which is probably beyond the scope for a simple class like ArrayView.

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

I am not sure if it is a good idea to make StridedArrayView a base class of ArrayView. StridedArrayView looks like a variant of a view; there are other views like in the case of slices of python.

I know why you want to do this. You want to simply extract the strides in FPE.

Personally, I would prefer to have StridedArrayView and ArrayView and accept both classes in FPE and decide internally via const expressions what kind of strides to use. This is just a bit more code in FPE.

@bergbauer
Copy link
Contributor Author

Personally, I would prefer to have StridedArrayView and ArrayView and accept both classes in FPE and decide internally via const expressions what kind of strides to use. This is just a bit more code in FPE.

This would of course also be possible but I think StridedArrayView is a generalization where ArrayView is the special case with stride = 1. The question is if StridedArrayView can be useful in other parts of the library. If yes, I would go with the in this PR proposed solution, if no I can also implement a special solution for FPE. What do others think?

@peterrum
Copy link
Member

StridedArrayView is a generalization where ArrayView is the special case with stride = 1. The question is if StridedArrayView can be useful in other parts of the library.

This is one point of view. The other one is a filtered vector, where all entries are "enabled".

@bergbauer
Copy link
Contributor Author

Can we come to a decision here? I would like to move forward with #15971

@kronbichler
Copy link
Member

The question is if StridedArrayView can be useful in other parts of the library

I do not see an immediate use, but one could imagine several potential uses in some parts (e.g. for tensor-product evaluation). But I think someone else should decide.

@kronbichler
Copy link
Member

It seems there are no statements in support. Thus I suggest to solve the problem within FEPointEvaluation for now as suggested by @peterrum. We can close this for now but come back in case there is support for this PR. Does this make sense @bergbauer?

@bergbauer
Copy link
Contributor Author

What about adding it as a parallel class (and not as base class of ArrayView, see last commit)?

Base class makes more sense for me because of the code duplication with this solution but this would also work.

@bangerth
Copy link
Member

I don't have a particularly strong opinion about this patch, but would like to point out that we document ArrayView as comparable to the C++20 class std::span: https://en.cppreference.com/w/cpp/container/span . std::span explicitly mentions that it represents a contiguous range.

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

This looks ok for the present application.

@kronbichler
Copy link
Member

/rebuild

@peterrum peterrum dismissed bangerth’s stale review September 17, 2023 12:33

Changes have been applied.

@peterrum
Copy link
Member

Let's merge so that @bergbauer can proceed with #15971.

@peterrum peterrum merged commit 170a1ef into dealii:master Sep 17, 2023
15 checks passed
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

5 participants