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

clean up profiles #175

Merged
merged 9 commits into from
Oct 10, 2024
Merged

clean up profiles #175

merged 9 commits into from
Oct 10, 2024

Conversation

cqc-melf
Copy link
Collaborator

@cqc-melf cqc-melf commented Oct 10, 2024

Description

Clean up and first steps of adding support for the base profile

This PR is moving all the code used by all profiles from the two QirGenerator classes to an abstract class.

Adding support for ADAPTIVE_CREGSIZE option which is generating profile compatible QIR with assuring the registers size of the integers representing them. This is not yet working on the hardware, so I have not added that to the changelog.

I will add the changes for the base profile in a second PR.

It might be easier to review the commits, they are structures to keep the renaming and changes in the QIR files separated from the code changes.

Related issues

Solves #168

Checklist

  • I have performed a self-review of my code.
  • I have commented hard-to-understand parts of my code.
  • I have made corresponding changes to the public API documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the changelog with any user-facing changes.

@cqc-melf cqc-melf marked this pull request as ready for review October 10, 2024 11:33
https://github.com/qir-alliance/qir-spec/tree/main/specification/under_development/profiles/Adaptive_Profile.md
Use QIRProfile.ADAPTIVE_CREGSIZE for the adaptive profile with additional
truncation operation to assure that intergers matching the classical
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
truncation operation to assure that intergers matching the classical
truncation operation to assure that integers matching the classical

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -112,7 +112,7 @@
}


class QirGenerator:
class AbsQirGenerator:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought of abstract which is have now written fully. Open to other suggestions.

Comment on lines 15 to 18
"""
This module contains all functionality to generate QIR files
from pytket circuits.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace these "module-level" descriptions with "file-level" descriptions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is not really helpful at the moment, I have remove this and added more details to the docs strings of the classes in the files. Maybe that is better?

)


class AProfileQirGenerator(AbsQirGenerator):
Copy link
Collaborator

Choose a reason for hiding this comment

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

A for "adaptive"? I think using the full name would be better -- and also changing the filename to reflect this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is for Adaptive, I have expended this and added some more details to the docstring of the class.

pytket/qir/conversion/pytketqirgenerator.py Outdated Show resolved Hide resolved
pytket/qir/conversion/qirgenerator.py Outdated Show resolved Hide resolved
cqc-melf and others added 2 commits October 10, 2024 14:41
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
@cqc-melf cqc-melf merged commit bc05c12 into main Oct 10, 2024
6 checks passed
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.

2 participants