Skip to content

Add precision log in apply with profiler_hook#1921

Merged
yhmtsai merged 4 commits intodevelopfrom
log_apply_type
Dec 5, 2025
Merged

Add precision log in apply with profiler_hook#1921
yhmtsai merged 4 commits intodevelopfrom
log_apply_type

Conversation

@yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Aug 21, 2025

This PR allows profiler hook to log the precision in apply which can help checking the mixed precision setup.

I am thinking whether to have Dense/Vector in that, which might be useful when identify it is local solver or distributed solver in distributed setup

The current format will be like Dense, Vector, or linop
spmv: obj*input_type=output_type
advanced: alpha_type*obj*input_type+beta_type*output_type

@yhmtsai yhmtsai requested review from a team August 21, 2025 07:37
@yhmtsai yhmtsai self-assigned this Aug 21, 2025
@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Aug 21, 2025
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. labels Aug 21, 2025
@MarcelKoch MarcelKoch added this to the Ginkgo 1.11 milestone Nov 13, 2025
Comment on lines +191 to +195
ss << "apply(" << stringify_object(A);
if (log_apply_precision_) {
ss << "*" << check_vector_type(b) << "=" << check_vector_type(x);
}
ss << ")";
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe just put the in- and output vector as arguments to the apply, i.e.:

Suggested change
ss << "apply(" << stringify_object(A);
if (log_apply_precision_) {
ss << "*" << check_vector_type(b) << "=" << check_vector_type(x);
}
ss << ")";
ss << "apply(" << stringify_object(A) << ", " << check_vector_type(b) << ", " << check_vector_type(x) << ")";

same at the other instances

Copy link
Member Author

Choose a reason for hiding this comment

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

I will still put the scalar type for log because the mismatched type will trigger temporary clone

@yhmtsai yhmtsai requested a review from MarcelKoch December 4, 2025 07:29
Copy link
Member

@MarcelKoch MarcelKoch 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 the output still needs some discussion, especially how it interacts with stringify_objects.

Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe remove this.

Copy link
Member Author

Choose a reason for hiding this comment

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

could you point where should be removed? github does not show the lines

Copy link
Member

Choose a reason for hiding this comment

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

This file only has the license date changed. Nothing else.

namespace {


std::string check_vector_type(const LinOp* linop)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should better be merged with stringify_objects. If a user explicitly sets a name for an object, it should override this behavior here.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, isn't stringify_objects already enough?

Copy link
Member Author

@yhmtsai yhmtsai Dec 4, 2025

Choose a reason for hiding this comment

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

Good point!
I try it and it works smoothly.
only some nit: it contains the full name like gko::matrix::...
for complex, it gives additional space like gko::matrix::Dense<std::complex<double> >

@yhmtsai yhmtsai requested a review from MarcelKoch December 4, 2025 09:37
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Dec 4, 2025
@yhmtsai yhmtsai merged commit 6817858 into develop Dec 5, 2025
22 of 25 checks passed
@yhmtsai yhmtsai deleted the log_apply_type branch December 5, 2025 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. reg:testing This is related to testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants