Skip to content

1888/rename-dense - #2064

Open
MarcelKoch wants to merge 4 commits into
developfrom
1888/rename-dense
Open

1888/rename-dense#2064
MarcelKoch wants to merge 4 commits into
developfrom
1888/rename-dense

Conversation

@MarcelKoch

@MarcelKoch MarcelKoch commented Aug 17, 2026

Copy link
Copy Markdown
Member

This PR renames Dense to MultiVector

@MarcelKoch MarcelKoch added this to the Ginkgo 2.0 milestone Aug 17, 2026
@MarcelKoch MarcelKoch self-assigned this Aug 17, 2026
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:benchmarking This is related to benchmarking. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats type:factorization This is related to the Factorizations type:reordering This is related to the matrix(LinOp) reordering type:multigrid This is related to multigrid type:stopping-criteria This is related to the stopping criteria mod:all This touches all Ginkgo modules. labels Aug 17, 2026
@MarcelKoch MarcelKoch mentioned this pull request Aug 17, 2026
Open
9 tasks
yhmtsai
yhmtsai requested changes Aug 18, 2026
View reviewed changes
Comment thread include/ginkgo/core/matrix/multivector.hpp Outdated
Comment thread include/ginkgo/core/matrix/multivector.hpp Outdated
Comment thread include/ginkgo/core/matrix/multivector.hpp Outdated
Comment thread common/cuda_hip/factorization/par_ilut_spgeam_kernels.cpp
#include <ginkgo/core/base/math.hpp>
#include <ginkgo/core/matrix/coo.hpp>
#include <ginkgo/core/matrix/csr.hpp>
#include <ginkgo/core/matrix/dense.hpp>

@yhmtsai yhmtsai Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

only renaming the header without changing anything in the rest sounds we do not need this header

@yhmtsai yhmtsai Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same for the other file only renaming header

@MarcelKoch MarcelKoch Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The header also include device_views.hpp which is actually used here. So I replaced the includes

Comment thread core/base/dispatch_helper.hpp
#include <memory>

#include <ginkgo/core/base/exception_helpers.hpp>
#include <ginkgo/core/matrix/multivector.hpp>

@yhmtsai yhmtsai Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
#include <ginkgo/core/matrix/multivector.hpp>

do we need that?

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is still unresolved ? I dont think this header include is needed ?

Comment thread include/ginkgo/core/base/combination.hpp Outdated
Comment thread test/mpi/distributed/vector.cpp
Comment thread include/ginkgo/core/base/precision_dispatch.hpp
Comment on lines +32 to +33
* matrix::MultiVector<ValueType>, otherwise it will be converted
* to this type if possible.

@yhmtsai yhmtsai Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* matrix::MultiVector<ValueType>, otherwise it will be converted
* to this type if possible.
* matrix::MultiVector<ValueType>, otherwise it will be converted
* to this type if possible.

Comment thread include/ginkgo/core/base/precision_dispatch.hpp Outdated
Comment thread include/ginkgo/core/matrix/multivector.hpp
@@ -86,15 +87,15 @@ class SparsityCsr;
* @ingroup LinOp
*/
template <typename ValueType = default_precision>
class Dense
class MultiVector

@yhmtsai yhmtsai Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

because we will merge one by one, I prefer adding an alias until Dense reintroduced is merged

template <typename ValueType = default_precision>
using Dense = MultiVector<ValueType>;

but only add it when all ci are passed

@MarcelKoch MarcelKoch Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just FYI, it will not be possible to have all PRs in this stack pass their CI. For this PR though your suggestion should work.

@MarcelKoch
MarcelKoch requested a review from yhmtsai August 18, 2026 12:59
pratikvn
pratikvn requested changes Aug 20, 2026
View reviewed changes

@pratikvn pratikvn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some comments

Comment thread reference/test/matrix/batch_dense_kernels.cpp
protected:
using value_type = T;
using size_type = gko::size_type;
using BMtx = gko::batch::matrix::Dense<value_type>;
using BMtx = gko::batch::matrix::MultiVector<value_type>;

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These seem to be wrongly substituted. For batched files, you can maybe just skip the substitutions for now, and do targeted substitutions in a second pass ?

Comment thread include/ginkgo/core/matrix/multivector.hpp
@@ -71,7 +71,8 @@ class SparsityCsr;


/**

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I would prefer this file to be move into include/ginkgo/core/base/ rather than in core/matrix/, similar to batch::MultiVector

Comment thread include/ginkgo/core/matrix/multivector.hpp
@@ -1520,22 +1534,27 @@ class Dense
};


template <typename ValueType>

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be

Suggested change
template <typename ValueType>
template <typename ValueType = default_precision>

Comment thread include/ginkgo/core/matrix/multivector.hpp
using transposed_type = Dense<value_type>;
using mat_data = matrix_data<value_type, int64>;
using transposed_type = MultiVector<value_type>;
using mat_data64 = matrix_data<value_type, int64>;

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are the mat_data and device_mat_data intentional renames ?

@MarcelKoch MarcelKoch Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, for consistency

Comment thread core/distributed/dense_communicator.cpp
#define GKO_DECLARE_DENSE_CONSTRUCTOR(LocalIndexType, GlobalIndexType) \
DenseCommunicator::DenseCommunicator( \
communicator base, \
#define GKO_DECLARE_MULTIVECTOR_CONSTRUCTOR(LocalIndexType, GlobalIndexType) \

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Incorrect substitutions in this file.

Comment thread core/base/dispatch_helper.hpp
#include <memory>

#include <ginkgo/core/base/exception_helpers.hpp>
#include <ginkgo/core/matrix/multivector.hpp>

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is still unresolved ? I dont think this header include is needed ?

Comment thread include/ginkgo/core/base/perturbation.hpp
Comment on lines +15 to +22
namespace matrix {


template <typename ValueType>
class MultiVector;


}

@pratikvn pratikvn Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you need the multivector.hpp header included here instead of the forward decl, as you use the complete type below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pratikvn pratikvn pratikvn requested changes
@yhmtsai yhmtsai Awaiting requested review from yhmtsai

Requested changes must be addressed to merge this pull request.

Assignees

@MarcelKoch MarcelKoch

Labels

1:ST:ready-for-review This PR is ready for review 1:ST:skip-full-test mod:all This touches all Ginkgo modules. reg:benchmarking This is related to benchmarking. reg:build This is related to the build system. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers type:stopping-criteria This is related to the stopping criteria

Projects

None yet

Milestone

Ginkgo 2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@MarcelKoch @pratikvn @yhmtsai @ginkgo-bot