New multirep PR initiated #496
Conversation
arsalan-motamedi
added
the
no changelog
| @@ -325,6 +325,11 @@ def indices(self) -> tuple[int, ...]: | |||
| self.index_dicts[t][m] for t, modes in enumerate(self.sorted_args) for m in modes | |||
| ) | |||
|
|
|||
|
|
|||
| @cached_property | |||
There was a problem hiding this comment.
Why are we caching this? This seems unnecessary as it's just a reordering of an already cached property i.e. duplicate information being cached
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #496 +/- ##
===========================================
+ Coverage 89.77% 89.88% +0.11%
===========================================
Files 104 104
Lines 7639 7871 +232
===========================================
+ Hits 6858 7075 +217
- Misses 781 796 +15
Continue to review full report in Codecov by Sentry.
|
…d btops has inverse representation now)
|
|
||
| # add some validation | ||
| self._index_representation_ = value | ||
|
|
||
| def _serialize(self) -> tuple[dict[str, Any], dict[str, ArrayLike]]: |
There was a problem hiding this comment.
Do you need to incorporate _index_representation into the serialization of the object?
There was a problem hiding this comment.
Hmm.. not sure about that. But I guess it makes some sense to keep _index_representationas a hidden attribute since there are many calls to circuit component initializer and I might be missing correcting some when doing the change.
| rep = self.representation + other.representation | ||
| rep = ( | ||
| self.to_bargmann().representation + other.to_bargmann().representation | ||
| ) # addition occurs in bargmann always |
There was a problem hiding this comment.
Why do we need it in bargmann?
There was a problem hiding this comment.
It is to avoid adding things with different representations. Lmk if you have a better approach in mind.
| ): | ||
| return self.representation == other.representation and self.wires == other.wires | ||
| else: | ||
| self_rep = self.to_bargmann().representation |
There was a problem hiding this comment.
Not sure how I feel about this why do we need this change?
There was a problem hiding this comment.
Well.. two things are equal if they are equal in the same representation. I mean, in physics, we call them the same thing.
There was a problem hiding this comment.
But why do we need to call to_bargmann in the __eq__? I would expect it to just be
def __eq__(self, other):
if isinstance(other, CircuitComponent):
return self.representation == other.representation and self.wires == other.wires
return False
|
|
||
| def __mul__(self, other: Scalar) -> CircuitComponent: | ||
| r""" | ||
| Implements the multiplication by a scalar from the right. | ||
| """ | ||
| return self._from_attributes(self.representation * other, self.wires, self.name) | ||
| ret = self._from_attributes(self.representation * other, self.wires, self.name) | ||
| ret._index_representation = self._index_representation |
There was a problem hiding this comment.
I am not sure I understand the question. Can you please elaborate?
There was a problem hiding this comment.
Same issue you encountered before where the solution was to deepcopy the dictionary. As in, ret._index_representation is now pointing to the dictionary of self._index_representation so any mutations to the latter propagate to the former
| d2 = {mode + len(modes): ("B", None) for mode in range(len(modes))} | ||
| d3 = {mode + 2 * len(modes): ("PS", float(self.s.value)) for mode in range(len(modes))} | ||
| d4 = {mode + 3 * len(modes): ("B", None) for mode in range(len(modes))} | ||
| self._index_representation = {**d1, **d2, **d3, **d4} |
There was a problem hiding this comment.
Alternatively now that self._index_representation is initialized in the super().__init__ you can just update the output indices
| @@ -16,6 +16,7 @@ | |||
| The class representing an operation that changes Bargmann into quadrature. | |||
| """ | |||
| # pylint: disable=protected-access | |||
| # pylint: disable=protected-access | |||
There was a problem hiding this comment.
| # pylint: disable=protected-access |
| self._add_parameter(Constant(phi, "phi")) | ||
| self.phi = phi | ||
| d1 = {mode: ("Q", float(self.phi.value)) for mode in range(len(modes))} | ||
| d2 = {mode + len(modes): ("B", None) for mode in range(len(modes))} |
There was a problem hiding this comment.
Ditto to what I mentioned in BtoPS
| if isinstance(self.representation, Bargmann): # TODO: better name for Bargmann class | ||
| # check cc rep | ||
| if not indices: | ||
| indices = self.wires.indices |
There was a problem hiding this comment.
| indices = self.wires.indices | |
| indices = indices if indices else self.wires.indices |
Context:
We need a way to handle representations, without needing to worry about them when taking contractions.
Description of the Change:
The main change is adding
_index_representationas an attribute toCircuitComponent, which keeps track of representation on each index.Benefits:
We don't need to worry about
BtoQandBtoPSchanging the result of contractions.Possible Drawbacks:
All contractions are computed in Bargmann still. We may want to enforce contacting in a specific representation.
Related GitHub Issues:
None.