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.

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

Refactoring the Qasm3Visitor #142

Merged
merged 10 commits into from
Aug 22, 2024
Merged

Refactoring the Qasm3Visitor #142

merged 10 commits into from
Aug 22, 2024

Conversation

TheGupta2012
Copy link
Collaborator

@TheGupta2012 TheGupta2012 commented Aug 15, 2024

Fixes #136

Summary of changes

  • With the addition of new constructs in the visitor, a refactoring was needed for the module
  • Validators, helpers and some other methods were factored out of the Qasm3Visitor. These were independent of the self and moved out to a separate utility class. Moreover, it made sense to keep all the _visit_* methods in the visitor.
  • Imports for openqasm3 were also moved to a different file
  • One possible separation could also be done based on the constructs i.e. GateUtils, VariableUtils, etc. but felt a bit of an overkill

@ryanhill1 would like to hear your thoughts on this too, thanks!

Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 97.44409% with 8 lines in your changes missing coverage. Please review.

Files Patch % Lines
qbraid_qir/qasm3/expressions.py 95.23% 4 Missing ⚠️
qbraid_qir/qasm3/validator.py 97.91% 2 Missing ⚠️
qbraid_qir/qasm3/exceptions.py 88.88% 1 Missing ⚠️
qbraid_qir/qasm3/transformer.py 98.38% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@TheGupta2012 TheGupta2012 self-assigned this Aug 19, 2024
Copy link
Member

@ryanhill1 ryanhill1 left a comment

Choose a reason for hiding this comment

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

  1. Let's remove the qasm3.utils.imports file and instead use namespace imports in qasm3.visitor and other files that have big import blocks.
  2. Please refactor to eliminate any ascending relative imports. This can be done either by moving all of files within qasm3.utils up to the qasm3 module level, or potentially with some clever inversion of the module structure. In the former, there would still only be 8 python files within the top-level qasm3 module, which isn't that bad.

See comments below.

qbraid_qir/qasm3/utils/maps.py Outdated Show resolved Hide resolved
qbraid_qir/qasm3/utils/imports.py Outdated Show resolved Hide resolved
qbraid_qir/qasm3/visitor.py Outdated Show resolved Hide resolved
Copy link
Member

@ryanhill1 ryanhill1 left a comment

Choose a reason for hiding this comment

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

Overall looking much better! Last few things to address which I mention in more detail below:

  • Making sure all functions have appropriate type hints (this will also help make Add static type checking with mypy #145 easier)
  • Combine in the print_err_location and exception raising by either modifying the existing Qasm3ConversionError exception class or creating a helper function that both raises the exception and logs/prints the element line/column traceback.
  • Rename the visitor_utils file and Qasm3VisitorUtils class to something a bit more descriptive. Regroup validators / analyzers / transformers as necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Please add type hints to all methods to comply with py.typed

qbraid_qir/qasm3/visitor_utils.py Outdated Show resolved Hide resolved
qbraid_qir/qasm3/visitor_utils.py Outdated Show resolved Hide resolved
qbraid_qir/qasm3/visitor_utils.py Outdated Show resolved Hide resolved
@TheGupta2012
Copy link
Collaborator Author

TheGupta2012 commented Aug 22, 2024

Copy link
Member

@ryanhill1 ryanhill1 left a comment

Choose a reason for hiding this comment

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

These updates look great! If you could just address the two lines of unreachable code I noted below either by updating the typehint, or removing them. After that it should be good

Edit: Also, is the supported conversions table up to date? And if not, could you update it? Thanks!

qbraid_qir/qasm3/analyzer.py Show resolved Hide resolved
qbraid_qir/qasm3/transformer.py Show resolved Hide resolved
@TheGupta2012
Copy link
Collaborator Author

Sure, will change. Also, some type hints are still left but I'll address them in the mypy issue

@ryanhill1 ryanhill1 merged commit 1ca2d93 into main Aug 22, 2024
7 checks passed
@ryanhill1 ryanhill1 deleted the refactor-qasm branch October 18, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor the BasicQasmVisitor
2 participants