Give create_submatrix a virtual behavior.

[tcojean]

This is useful for users to be able to derive from Dense in order to
specify the behavior of our solvers (i.e., GMRES).

[tcojean]

My understanding of this issue is that for the purpose of @nbeams integration of Ginkgo with MFEM it should be enough to copy/paste the implementation of create_submatrix_impl into her own wrapper since no actual allocation or anything specific is required, all she wants is to obtain the proper type (her type).

A bigger question is whether this simple virtual function is enough for the MPI implementation as well.

[yhmtsai]

LGTM.
Maybe I remember something wrong or there is already alternative way to work.
gmres also needs create_with_same_config_of for more columns.

[pratikvn]

Would this be enough ? Is there already a definition of span that can be used externally ? If not we can provide an additional function with expanded arguments (row_start, row_end, col_start , col_end) instead of a span ?

[nbeams]

Maybe I remember something wrong or there is already alternative way to work.
gmres also needs create_with_same_config_of for more columns.

I agree with @yhmtsai - as I understand it, I don't think this change will be enough for the MFEM integration. In GMRES,
create_submatrix is called on krylov_bases, which is created directly as a Dense of a certain size (so will not know anything about the VectorWrapper type or have a pointer to anything MFEM can understand). Then even if create_submatrix is virtual, it won't matter.

I suppose the two-step approach of "create multiple with config of" + "virtual create sub-operator" isn't the only possible option, though. For example, there could be a virtualcreate_submatrix_with_config_of which could be called on, say, the rhs vector (which will be of VectorWrapper type if necessary for MFEM) and also take the Dense object (krylov_bases in this case) as a parameter. In the MFEM case, the virtual function would be responsible for accessing a specific portion of the Dense matrix's data, creating an MFEM Vector that points to that data, and then using it to create a VectorWrapper which is returned from the function. Then I think this would be the only extra function we would need (krylov_bases could still be created from Dense directly). However in this case specifically, the data for the submatrix would have to contiguous (since an MFEM Vector must have contiguous data). Also I'm not sure this would be a better approach, merely that I think maybe it could be possible?

[tcojean]

I think the span shouldn't be an issue, it's available in the public header range.hpp.

But @yhmtsai and @nbeams are right that GMRES needs to change as well and a new function needs to be created such that hessenberg and krylov_basis can be initialized in a polymorphic fashion as well.

For that, it looks like all we need is a function which takes in a dim<2> parameter. Is it sufficient/OK if I simply add something like a create_with_type(other_object, dim<2>) or something like that which would imitate Dense::create() but be use another Dense object to reuse its type (and executor), take and take a dim<2> parameter? It would similarly use a create_with_type_impl() which can be overloaded to return the proper type.

[nbeams]

So from the MFEM integration viewpoint specifically, we would have:

• Due to the fact that VectorWrapper inherits from Dense, anything returned by the create_virtual function will be of VectorWrapper type and must also be able to be understood by Ginkgo as a Dense. Thus we can't actually create an "array of VectorWrappers", or n separate VectorWrappers of size m, which we could then pick out one at a time. Instead, we will have to create "one big VectorWrapper" of size m*n.
• Then, when doing create_submatrix, we will proceed as mentioned in my previous comment, by getting pointers to different places within the data of the big VectorWrapper/Dense, creating a new MFEM Vector of size m pointing to that data starting point, then creating a VectorWrapper from this Vector.

Is this correct?

[codecov]

Codecov Report

Merging #718 (bd83f1e) into develop (3e31cc0) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           Coverage Diff            @@
##           develop     #718   +/-   ##
========================================
  Coverage    92.40%   92.40%           
========================================
  Files          380      380           
  Lines        27485    27494    +9     
========================================
+ Hits         25397    25407   +10     
+ Misses        2088     2087    -1     

Impacted Files	Coverage Δ
include/ginkgo/core/matrix/dense.hpp	96.52% <86.66%> (-1.61%)	⬇️
core/solver/gmres.cpp	100.00% <100.00%> (ø)
omp/reorder/rcm_kernels.cpp	97.53% <0.00%> (+1.87%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e31cc0...bd83f1e. Read the comment docs.

[tcojean]

Your assumptions are correct Natalie, but I think you don't need to actually "create an MFEM vector" in the create_submatrix_impl, what this does is only return a Dense which reuses the same data but through a different shape. Hopefully you can simply copy/paste the code that is in Dense for that function except you would create a VectorWrapper instead. All you need is that it uses the VectorWrapper type I think right? MFEM doesn't need to see that part necessarily (I think?)

[tcojean]

format!

[nbeams]

No, I do need to create an MFEM Vector. The problem right now happens when I have an MFEM matrix-free operator, which means MFEM will handle the matvec itself, in which case I need an MFEM Vector.

[upsj]

LGTM!

[tcojean]

If @nbeams can confirm that technically there is everything needed for the purpose of the MFEM-Ginkgo integration, I think this would be ready to be merged.

[thoasm]

LGTM!

[pratikvn]

LGTM!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

16.7% Coverage
0.0% Duplication