Skip to content

1912/0 identity config #1917

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

1912/0 identity config #1917

wants to merge 1 commit into from

Conversation

MarcelKoch
Copy link
Member

This PR allows parsing an object of type matrix::Identity. This is helpful for configuring the preconditioner benchmark with no preconditioner.

@MarcelKoch MarcelKoch self-assigned this Aug 19, 2025
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats labels Aug 19, 2025
@MarcelKoch MarcelKoch mentioned this pull request Aug 15, 2025
Open
6 tasks
@yhmtsai
Copy link
Member

yhmtsai commented Aug 20, 2025

Doesn't null work?
By the way, maybe we should have preconditioner::Identity to make clear?

yhmtsai
yhmtsai requested changes Aug 25, 2025
View reviewed changes
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

just realize we already have matrix Identity, so no need for preconditioner::Identity

pratikvn
pratikvn approved these changes Aug 27, 2025
View reviewed changes
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

lgtm

@MarcelKoch MarcelKoch force-pushed the 1912/0-identity-config branch from a0d77bd to 02b84db Compare September 29, 2025 10:35
@MarcelKoch MarcelKoch requested a review from yhmtsai September 29, 2025 10:35
@ginkgo-bot
Copy link
Member

Error: The following files need to be formatted:

cuda/base/exception.cpp
dpcpp/factorization/par_ilut_select_kernel.dp.cpp
include/ginkgo/extensions/kokkos/spaces.hpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

upsj
upsj approved these changes Sep 29, 2025
View reviewed changes
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM!

core/config/config_helper.hpp
Pgm,
Schwarz
Schwarz,
Identity
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
Identity
Identity,

I think C++ allows trailing commas, that makes the diffs nicer

yhmtsai
yhmtsai requested changes Oct 9, 2025
View reviewed changes
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

one extra header

core/test/config/matrix.cpp
#include <ginkgo/core/config/config.hpp>
#include <ginkgo/core/matrix/identity.hpp>

#include "cmake-build-debug/_deps/googletest-src/googletest/include/gtest/gtest-typed-test.h"
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
#include "cmake-build-debug/_deps/googletest-src/googletest/include/gtest/gtest-typed-test.h"

do you need this?

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

Reviewers

@yhmtsai yhmtsai yhmtsai requested changes

@upsj upsj upsj approved these changes

@pratikvn pratikvn pratikvn approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@MarcelKoch MarcelKoch

Labels

mod:core This is related to the core module. reg:build This is related to the build system. reg:testing This is related to testing. type:matrix-format This is related to the Matrix formats

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MarcelKoch @yhmtsai @ginkgo-bot @upsj @pratikvn