-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Move CircuitDag to contrib #5481
Move CircuitDag to contrib #5481
Conversation
- Moves CircuitDag to contrib. - cirq.CircuitDag is now deprecated and now must be changed to cirq.contrib.CircuitDag. - Also moves the related cirq.Unique class Note: the tests use a high number of cirq.Unique instances which are also moved, so unable to verify count of deprecation messages in many tests.
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.
LGTM after a few tweaks. Mostly module import issues. Also, do we want to create a whole new folder in contrib or just copy the two modules over without the new folder ?
|
||
from cirq.contrib.acquaintance.executor import AcquaintanceOperation, ExecutionStrategy | ||
from cirq.contrib.acquaintance.mutation_utils import expose_acquaintance_gates | ||
from cirq.contrib.acquaintance.permutation import LogicalIndex, LogicalMapping | ||
from cirq.contrib.circuitdag import CircuitDag |
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: prefer module 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.
Done.
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from cirq.contrib.circuitdag.circuit_dag import CircuitDag, Unique |
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.
Why do we need our own circuitdag
folder ?
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's used in 3 different sub modules - contrib/acquaintance; contrib/routing; contrib/paulistring
So, I think it makes sense to make a separate submodule / folder for circuit dag. Note that all code is in specific submodules in contrib/* and we don't have any top-level python files in contrib, so we shouldn't put circuitdag as a top level python file directly in contrib/.
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.
Yeah, this was to stay consistent with other modules in contrib.
@@ -15,6 +15,7 @@ | |||
from typing import cast | |||
|
|||
from cirq import circuits, ops, protocols | |||
from cirq.contrib.circuitdag import CircuitDag |
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: module 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.
Done.
@@ -15,10 +15,11 @@ | |||
import networkx | |||
|
|||
from cirq import circuits, linalg | |||
from cirq.contrib.circuitdag import CircuitDag |
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: module 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.
Done.
@@ -15,7 +15,7 @@ | |||
from typing import Any, Callable, Iterable, Sequence, Tuple, Union, cast, List | |||
|
|||
from cirq import circuits, ops, protocols | |||
|
|||
from cirq.contrib.circuitdag import CircuitDag, Unique |
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.
Module 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.
Done.
@@ -31,6 +31,7 @@ | |||
|
|||
from cirq import circuits, ops, value | |||
import cirq.contrib.acquaintance as cca | |||
from cirq.contrib.circuitdag import CircuitDag |
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: module 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.
Done.
* Move CircuitDag to contrib - Moves CircuitDag to contrib. - cirq.CircuitDag is now deprecated and now must be changed to cirq.contrib.CircuitDag. - Also moves the related cirq.Unique class Note: the tests use a high number of cirq.Unique instances which are also moved, so unable to verify count of deprecation messages in many tests.
* Move CircuitDag to contrib - Moves CircuitDag to contrib. - cirq.CircuitDag is now deprecated and now must be changed to cirq.contrib.CircuitDag. - Also moves the related cirq.Unique class Note: the tests use a high number of cirq.Unique instances which are also moved, so unable to verify count of deprecation messages in many tests.
to cirq.contrib.CircuitDag.
Note: the tests use a high number of cirq.Unique instances which
are also moved, so unable to verify count of deprecation messages
in many tests.
Fixes: #3816