-
Notifications
You must be signed in to change notification settings - Fork 98
WIP: Multivector interface #1889
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
Draft
MarcelKoch
wants to merge
18
commits into
develop
Choose a base branch
from
1888/interface
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this mixin automatically casts the parameters to the concrete type.
Error: The following files need to be formatted:
You can find a formatting patch under Artifacts here or run |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
mod:core
This is related to the core module.
reg:build
This is related to the build system.
type:matrix-format
This is related to the Matrix formats
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a generic interface for (multi-)vector types. The idea is to unify the handling of our
Dense
anddistributed::Vector
classes. Additionally, it could allow users more easily to provide their own types to be used in our solvers.Notes on the design:
The multivector interface inherits from
LinOp
, since removing the inheritance fromLinOp
for our vectors is a significant change that would leave Ginkgo broken for a very long time. However, I would prefer to separate these two interfaces. (This might be a long term change, not even for v2.0.)The interface is completely generic; this especially means that it is not templated with a
ValueType
. One of the main issues regarding that is that the interface can't differentiate between complex and real vectors. I'm not sure if this is something we need to safeguard against at compile time, or if the checks at runtime will be sufficient. To remove the value type from places where our current interfaces rely on it (fill
,scale
,add_scaled
, ...) I resorted to usingstd::variant
. We always know the allowed value types (except for the complex/real issue), so I think it's reasonable to use a variant there.I also provide a mixin for the interface, which concretizes most of the functions again for the known derived type. The actual implementations of the interface should derive from the mixin. If they do so, then they only need to implement concretized functions. E.g. instead of implementing
compute_norm2_impl(MultiVector* result)
theDense<ValueType>
needs to implement onlycompute_norm2_impl(Dense<ValueType>::absolute_type*)
.I'm still in the process of figuring out how the precision dispatch can work. Right now I have the templated functions
std::unique_ptr<MultiVector> temporary_precision<ValueType>()
. I'm not sure if that is the approach we will pursue in the end.Since we pass the (local) vector to our kernels, I needed to add some functionality to provide a
std::unique_ptr<Dense<...>>
to the interface. TBH, I don't like that this is aDense
and I would prefer if we could directly return a device view ofDense
.