Skip to content

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Aug 18, 2025

This PR fixes the wrong condition with benchmark_from_scratch in #1881 .
It should generate the one explicitly when we do not want to measure timing from scratch

@yhmtsai yhmtsai requested a review from pratikvn August 18, 2025 08:08
@yhmtsai yhmtsai self-assigned this Aug 18, 2025
@yhmtsai yhmtsai added is:bugfix This fixes a bug 1:ST:ready-for-review This PR is ready for review labels Aug 18, 2025
@ginkgo-bot ginkgo-bot added reg:benchmarking This is related to benchmarking. type:solver This is related to the solvers and removed 1:ST:ready-for-review This PR is ready for review labels Aug 18, 2025
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.

The benchmark tests are failing. Is benchmark_from_scratch being enabled by default ? The comparison output needs to be updated then, I think

@yhmtsai
Copy link
Member Author

yhmtsai commented Aug 21, 2025

It is currently disabled by default -> the benchmark will reuse the same solver object
Thus, the output have additional setup because it will always construct another new for reusing

@yhmtsai yhmtsai force-pushed the fix_missing_benchmark branch from 5125175 to 710011c Compare August 29, 2025 16:14
Comment on lines -46 to -49
DEBUG: begin dense::fill
DEBUG: end dense::fill
DEBUG: begin dense::fill
DEBUG: end dense::fill
Copy link
Member Author

Choose a reason for hiding this comment

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

because we avoid reinitialization now

DEBUG: begin dense::copy
DEBUG: end dense::copy
DEBUG: end copy(<typename>)
DEBUG: begin generate(<typename>)
Copy link
Member Author

Choose a reason for hiding this comment

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

first run before repetition to ensure workspace and constant are ready

@yhmtsai yhmtsai requested a review from pratikvn August 29, 2025 16:17
@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Aug 29, 2025
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!

- removal because we do clone twice when from scratch
- addition because we perform first run ensuring workspace. repetition one are the same up to some initalization
@yhmtsai yhmtsai force-pushed the fix_missing_benchmark branch from 710011c to 631a010 Compare September 4, 2025 09:55
@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 Sep 4, 2025
@yhmtsai yhmtsai merged commit 49c8482 into develop Sep 4, 2025
17 of 19 checks passed
@yhmtsai yhmtsai deleted the fix_missing_benchmark branch September 4, 2025 14:15
Copy link

sonarqubecloud bot commented Sep 4, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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. is:bugfix This fixes a bug reg:benchmarking This is related to benchmarking. type:solver This is related to the solvers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants