-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add PLY Parser #84
Add PLY Parser #84
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work. I think the test code sets up an example for others.
One problem is mandatory to be fixed before we can merge this PR, that is you need to install qelib1.inc
, non-py files are automatically excluded when using pip install .
I also commented on the details.
BTW, please also include the license header for new files.
Other comments are just some general suggestions and you can igore them.
Besides, there're some common practice we can try to follow
- Split large PR into smaller ones to maker life easier for reviewers.
- For such a complete feature, I think a document in yuque is needed.
- We can set up an agreement on which functions need detailed comment, i.e.,
Args
,Returns
, etc. Typically APIs exposed to users need these instructions. For other small functions or internal member functions we don't need detailed comments.
def input(self, data): | ||
self.data = data | ||
self.lexer.input(data) | ||
# read qelib1.inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file qelib1.inc
needs to be installed through pip install .
You can refer to this link to install non-py files: https://setuptools.pypa.io/en/latest/userguide/datafiles.html
And this is the root cause of ut failure: https://github.com/ScQ-Cloud/pyquafu/actions/runs/6186274257/job/16793521132
You can also enable github actions in your forked repository, then you can test whether this issue has been fixed
break | ||
print(tok, file=print_file) | ||
|
||
if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest not to include these test codes in source file
] | ||
|
||
|
||
# 从最底层向上归约写 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to avoid chinese comments as it is opensource to the entire community
self.is_qubit = is_qubit | ||
self.qargs = [] | ||
self.cargs = [] | ||
self.instructions = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a blank line. Perhaps we can use black in the future.
self.filename = filename | ||
self.num = 0 | ||
|
||
class IndexedId(Node): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether it would be better to use dataclass for these Nodes.
Add PLY Parser and test for it.