-
Notifications
You must be signed in to change notification settings - Fork 217
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
[py][nfc] import
s clean up.
#2590
base: main
Are you sure you want to change the base?
Conversation
e997d80
to
77ba55a
Compare
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
77ba55a
to
2dbc748
Compare
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
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.
Good cleanup of imports! Well done following PEP8 principles.
Cleans up imports throughout the python runtime. I trie following the principles in PEP8 (https://peps.python.org/pep-0008/#imports). The changes include: * Grouping imports two blocks: external and internal, w.r.t cudaq. * Not using relative paths when the module is in a parent dir (`..`) * Not using wildcard imports * Removing unused ones Signed-off-by: boschmitt <[email protected]>
2dbc748
to
952d1e5
Compare
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
@@ -6,20 +6,36 @@ | |||
# the terms of the Apache License 2.0 which accompanies this distribution. # | |||
# ============================================================================ # | |||
import ast |
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.
nit: I think we are inconsistent with a blank line after the copyright banner and fist import.
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.
Indeed, we don't seem to have any guideline regarding these things.
from typing import get_origin | ||
import sys | ||
import traceback | ||
import numpy as np |
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.
Q: Are the external imports sorted alphabetically?
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.
I was going for that but it's really an arbitrary decision. The only exception being from __future__
which is required to be the first.
from .schedule import Schedule | ||
from .expressions import Operator | ||
from ..mlir._mlir_libs._quakeDialects import cudaq_runtime | ||
from cudaq.mlir._mlir_libs._quakeDialects import cudaq_runtime |
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.
Q: Is there relative ordering for the .
imports and named internal imports?
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.
It should have. The idea was to have project imports followed by relative imports. Also, I did change things in a way that relative imports are only for modules within the same directory (so moving the whole directory wouldn't require changing the imports)
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.
Minor comments. LGTM.
Thanks, Bruno! 👍🏽
@boschmitt : In case you used a tool / script for this, can you share. Perhaps we can add that as part of CI ? |
I did it by hand while studying the code, so it is really a best-effort sort of thing and is not really free of inconsistencies. Thanks for the review (: |
Description
Cleans up imports throughout the python runtime. I trie following the principles in PEP8.
The changes include:
..
)