Fix of implementation of SyrecSynthesis::increase operation #286

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

Merged

burgholzer merged 10 commits into munich-quantum-toolkit:main from TooMuchDakka:adder-fix

Jul 8, 2025

Collaborator

TooMuchDakka commented Jul 1, 2025 •

edited

Loading

Description

With this PR the counter-examples found in 🐛 Simulated results of line-aware synthesized circuit not matching expected values #254 can be correctly synthesized due to bugfixes in the SyrecSynthesis::increase operation. Additionally, further tests of the binary + operation as well as the assignment operations += and ++= are added that check that a simulation of these operations returns the correct results (the expected synthesis costs are not defined).
Various algorithms that can be used fix the errors of SyrecSynthesis::increase were proposed in 🐛 Simulated results of line-aware synthesized circuit not matching expected values #254 but since the pre-PR implementation already followed one of these algorithms (with the algorithm of "Quantum Addition Circuits and Unbounded Fan-Out" (Takahashi et al.) being the closest match) no complete reimplementation was needed.
Some simulation tests could not be added in the PR due to errors in the LineAwareSynthesis (e.g., see 🐛 Index out of range error when checking for overlapping qubit indices in operands of expression #280)
A breaking change in the internal SyrecSynthesis interface was added due to the unification of the SyrecSynthesis::increase and SyrecSynthesis::increaseWithCarry into one function.

Fixes #254

Checklist:

The pull request only contains commits that are related to it.
I have added appropriate tests and documentation.
I have made sure that all CI jobs on GitHub pass.
The pull request introduces no new warnings and follows the project's style guidelines.

TooMuchDakka and others added 7 commits

July 1, 2025 20:45


          Undocumented fix of increase operation of SyrecSynthesis

77ae0aa


          Updated documentation of the implementation of the SyrecSynthesis::in…

aea3394

…crease() operation


          Added further tests for simulation of addition binary operand and add…

fa57fb2

…/increment assignments


          Breaking change for public SyrecSynthesis interface by unification of…

3db0536

… increase and increaseWithCarry functions into one function


          Temporary removal of failing synthesis test due to existing bug in Li…

20a65fb

…neAwareSynthesis


          Removed a TODO comment

a3fb2d4


          🎨 pre-commit fixes

094f2ec

github-advanced-security bot found potential problems

View reviewed changes

github-advanced-security bot left a comment

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

codecov bot commented Jul 1, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/algorithms/synthesis/syrec_synthesis.cpp	60.0%	0 Missing and 6 partials ⚠️

📢 Thoughts on this report? Let us know!

Collaborator Author

TooMuchDakka commented Jul 1, 2025

@burgholzer This PR would be ready for a review if the required patch coverage was hit. Are there any recommendations for how we can increase the patch coverage of a PR where the offending lines are only partially covered (in which the not hit condition should only be triggered in exceptional cases, i.e. if a bug in the implementation exists) if we are only trying to test the public interfaces of the mqt-syrec library. The same issue is currently active in the PR #281.

Member

burgholzer commented Jul 3, 2025

Okay. I checked all the other PRs and merged where appropriate. Now this can be updated and should also be ready for review then.
Don't worry about the patch coverage if it should still be failing.

TooMuchDakka closed this

TooMuchDakka deleted the adder-fix branch

July 3, 2025 12:54

TooMuchDakka restored the adder-fix branch

July 3, 2025 13:09


          Merge of changes from main branch and refactoring of basic operation …

1ff824d

…simulationt tests to load data from json

Collaborator Author

TooMuchDakka commented Jul 3, 2025

Pull request was deleted and closed by mistake due to an incorrect deletion of a local branch thus the PR is reopened.

TooMuchDakka reopened this


          🎨 pre-commit fixes

e6dada8

TooMuchDakka added c++ fix labels

github-advanced-security bot found potential problems

View reviewed changes

test/unittests/simulation/test_synthesis_basic_operations.cpp

@@ @@ -59,6 +59,94 @@ @@
                   this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());
               }
+              TYPED_TEST_P(BaseSimulationTestFixture, AddAssignWithRightHandSideEqualToVariable) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_AddAssignWithRightHandSideEqualToVariable_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, AddAssignWithRightHandSideEqualToConstant) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_AddAssignWithRightHandSideEqualToConstant_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, AddAssignWithRightHandSideEqualToShiftExpression) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_AddAssignWithRightHandSideEqualToShiftExpression_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, AddAssignWithRightHandSideEqualToUnaryExpression) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_AddAssignWithRightHandSideEqualToUnaryExpression_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, AddAssignWithRightHandSideEqualToNestedExpression) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_AddAssignWithRightHandSideEqualToNestedExpression_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, AddAssignOfBitOfValueOfDimensionOfVariable) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_AddAssignOfBitOfValueOfDimensionOfVariable_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, IncrementAssignOfVariable) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_IncrementAssignOfVariable_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, IncrementAssignOfBitOfVariable) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_IncrementAssignOfBitOfVariable_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, IncrementValueOfDimensionOfVariable) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_IncrementValueOfDimensionOfVariable_defined_ is never read.

test/unittests/simulation/test_synthesis_basic_operations.cpp

+              }
+              TYPED_TEST_P(BaseSimulationTestFixture, IncrementBitOfValueOfDimensionOfVariable) {
+                  this->performTestExecutionForCircuitLoadedFromJson(RELATIVE_PATH_TO_TEST_CASE_DATA_JSON_FILE, this->getNameOfCurrentlyExecutedTest());

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_IncrementBitOfValueOfDimensionOfVariable_defined_ is never read.

TooMuchDakka marked this pull request as ready for review

July 3, 2025 17:42

Collaborator Author

TooMuchDakka commented Jul 8, 2025

Merging the changes made in this PR and #297 will result in a merge conflict and will need to be resolved manually after one of the PRs was review and merge into the main branch.

TooMuchDakka requested a review from burgholzer

July 8, 2025 15:47

burgholzer approved these changes

View reviewed changes

Member

burgholzer left a comment

LGTM 👍🏼 nice work on the documentation of the individual steps!


          Merge branch 'main' into adder-fix

d03ab76

burgholzer enabled auto-merge (squash)

July 8, 2025 16:16

Collaborator Author

TooMuchDakka commented Jul 8, 2025

@burgholzer Can a manual merge (via a squash of the commits) into the main branch be performed since the required patch coverage is/will not be reached by this PR?

burgholzer disabled auto-merge

July 8, 2025 17:28

burgholzer merged commit 611e72b into munich-quantum-toolkit:main

16 of 17 checks passed

TooMuchDakka deleted the adder-fix branch

July 19, 2025 17:47

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

Labels

c++ fix