Skip to content
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

Symbolic #161

Merged
merged 51 commits into from
Oct 21, 2022
Merged

Symbolic #161

merged 51 commits into from
Oct 21, 2022

Conversation

pehamTom
Copy link
Member

@pehamTom pehamTom commented Oct 5, 2022 •
edited by burgholzer
Loading

Description

This PR introduced functionality for equivalence checking of parametrized circuits with QCEC. This PR is related to cda-tum/mqt-core#199 and cda-tum/zx#15

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.

pehamTom and others added 16 commits July 22, 2022 16:30
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 10, 2022
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Oct 10, 2022

This pull request introduces 1 alert when merging be0e5e0 into cfe9212 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 17, 2022
View reviewed changes
burgholzer
burgholzer requested changes Oct 17, 2022
View reviewed changes
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

I just quickly went through all the changes here. Overall this looks great already.
You can find a couple of minor or philosophical remarks inline.

One thing that is still missing here is the corresponding documentation.
I'd argue, that this should be a separate page just as the verifying the compilation flow page for the "User Guide" and corresponding API documentation on the developer side.

Last but not least, and just as a reminder, the reference to the corresponding paper is still missing in the README and the documentation.

@pehamTom
Fixed bug with assigning missing parameters.
9fceddd
@pehamTom
Added config for parameterized checking to other configs.
48f391c 
Furthermore I added another test and unified the naming convention from "symbolic" to "parameterized".
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 19, 2022
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Oct 19, 2022

This pull request introduces 2 alerts when merging 48f391c into f50d20b - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times

burgholzer
burgholzer requested changes Oct 20, 2022
View reviewed changes
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

I just quickly went through all the changes and added my remarks down below.

pehamTom added 15 commits October 21, 2022 11:07
@burgholzer burgholzer added feature New feature or request python Anything related to Python code c++ Anything related to C++ code labels Oct 21, 2022
@burgholzer burgholzer self-assigned this Oct 21, 2022
burgholzer
burgholzer approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Still found some really minor things while going over the code. I'll probably incorporate those over the weekend.

@burgholzer burgholzer merged commit 844731f into main Oct 21, 2022
@burgholzer burgholzer deleted the symbolic branch October 21, 2022 21:37
MarlQ pushed a commit to MarlQ/qcec that referenced this pull request Dec 4, 2022
@dependabot
Bump extern/pybind11 from b07975f to 88a1bb9 (cda-tum#161)
62a0ca4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@burgholzer burgholzer burgholzer approved these changes

Assignees

@burgholzer burgholzer

Labels
c++ Anything related to C++ code feature New feature or request python Anything related to Python code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pehamTom @burgholzer