-
Notifications
You must be signed in to change notification settings - Fork 31
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
Return more info about circuit separation during partitioning #304
base: main
Are you sure you want to change the base?
Conversation
@@ -221,14 +229,28 @@ def partition_problem( | |||
bases = [] | |||
i = 0 | |||
for inst in qpd_circuit.data: | |||
if isinstance(inst.operation, SingleQubitQPDGate): |
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.
We probably want to support these for wire cutting, but they are not well-handled as of this PR, so this ValueError is appropriate. We can remove it for the wire cutting PR, if necessary
circuit=circuit, partition_labels="AABB", observables=observables | ||
) | ||
bases = [cut_info.basis for cut_info in cuts] |
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.
This PR is a breaking change, users now need to get each basis from the CutInfo
object
Pull Request Test Coverage Report for Build 5467281062
💛 - Coveralls |
@garrison any comment on breaking change vs deprecating the old field? |
@@ -72,7 +72,7 @@ def reconstruct_expectation_values( | |||
for label, subobservable in observables.items(): | |||
if any(obs.phase != 0 for obs in subobservable): | |||
raise ValueError("An input observable has a phase not equal to 1.") | |||
subobservables_by_subsystem = observables | |||
subobservables_by_subsystem = observables # type: ignore |
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 believe this is a false positive from mypy. The variable is a dict[Hashable, Any]
, and the expression is a dict[str | int, Any]
. I believe the expression type lies in a subset of the variable type, which should be fine.
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.
https://mypy.readthedocs.io/en/stable/generics.html#variance-of-generic-types explains why dict
is considered an invariant type rather than a covariant type. But yes, I think ignoring this type check makes the most sense for now.
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.
Cool, since this is technically correct, this means we have "correct" type hints with respect to Hashable
and str | int
. All the functions correctly indicate what functionality they expect
This is the first step toward supporting an easier interface for updating parameters associated with cut gates.
In the current cutting partitioning workflow, users are returned a dictionary of subcircuits as well as the bases associated with each unique cut. Instead of only returning the bases, we should return which gates in which subcircuits each of the bases are associated with.
Once this information is available from
partition_problem
, implementing functions for manipulating the QPD gates in circuits will be more straightforward.I have also (hopefully) improved the type hinting. Only the functions which require the keys be sorted call for
str | int
in their type hints, and the others just expectHashable
.This is currently implemented as a breaking change, as the
PartitionedCuttingProblem.bases
field is now thecuts
field. Users should access the bases via the new field. Maybe we'd rather deprecate the old field. I don't really think it's necessary but am open to it