Cache Circuit properties between mutations#6322
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6322 +/- ##
=======================================
Coverage 97.83% 97.83%
=======================================
Files 1108 1108
Lines 96266 96333 +67
=======================================
+ Hits 94184 94251 +67
Misses 2082 2082
☔ View full report in Codecov by Sentry. |
maffoo
force-pushed
the
u/maffoo/circuit-freeze
branch
from
298750e to
f736079
Compare
|
Something to note is that there are a number of other circuit methods that could be optimized in the same way. For example, |
maffoo
force-pushed
the
u/maffoo/circuit-freeze
branch
from
18775c4 to
d346071
Compare
This simplifies working with frozen circuits and circuit identity by storing the `FrozenCircuit` instance returned by `Circuit.freeze()` and returning the same instance until it is "invalidated" by any mutations of the Circuit itself. Note that if we make additional changes to the Circuit implementation, such as adding new mutating methods, we will need to remember to add `self._frozen = None` statements to invalidate the frozen representation in those places as well. To reduce risk, I have put these invalidation statements immediately after any mutation operations on `self._moments`, even in places where some invalidations could be elided or pushed to the end of a method; it seems safer to keep these close together in the source code. I have also added an implementation note calling out this detail and reminding future developers to invalidate when needed if adding other `Circuit` mutations.
maffoo
force-pushed
the
u/maffoo/circuit-freeze
branch
from
d346071 to
f88b750
Compare
maffoo
changed the title
FrozenCircuit instance from Circuit.freeze() until mutatedCircuit properties between mutations
|
I updated this to cache various other circuit parameters as well. To avoid slowing down mutation methods too much, I've changed this to call the |
NoureldinYosri
left a comment
NoureldinYosri
left a comment
There was a problem hiding this comment.
Overall looks good, but I think we need tests of the form
for each property X that we cache
query property X -> which caches the result
for each method Y that mutates X
do Y
query X again and check that the result is correct
| else: | ||
| self.append(flattened_contents, strategy=strategy) | ||
|
|
||
| def _mutated(self) -> None: |
There was a problem hiding this comment.
shouldn't this also include _is_measurement and _parameter_names?
| Repeated calls to `.freeze()` will return the same FrozenCircuit | ||
| instance as long as this circuit is not mutated. | ||
| """ | ||
| from cirq.circuits import FrozenCircuit |
There was a problem hiding this comment.
nit: should this be a full import path cirq.circuits.frozen_circuit?
There was a problem hiding this comment.
This was copied from what we had in AbstractCircuit.freeze previously. I don't feel strongly either way, though. Will change to use full import path.
This caches various computed properties on `Circuit` so that they do not need to be recomputed when accessed if the circuit has not been mutated. Any mutations cause these properties to be invalidated so that they will be recomputed the next time they are accessed.
3 participants
This simplifies working with frozen circuits and circuit identity by storing the
FrozenCircuitinstance returned byCircuit.freeze()and returning the same instance until it is "invalidated" by any mutations of the Circuit itself. Note that if we make additional changes to the Circuit implementation, such as adding new mutating methods, we will need to remember to addself._frozen = Nonestatements to invalidate the frozen representation in those places as well. To make this easier to audit, I have put these invalidation statements immediately after any mutation operations onself._moments, even in places where some invalidations could be elided or pushed to the end of a method; it seems safer to keep these close together in the source code. I have also added an implementation note calling out this detail and reminding future developers to invalidate when needed if adding otherCircuitmutations.