Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cache tape executions #817
Cache tape executions #817
Changes from 14 commits
e653628
62f3582
9686447
67c7f61
625e3b7
5fea812
f084d9d
fe453d8
e6740ce
bf064a8
db32341
b1160bf
5bd1d37
f86adc7
4dbee74
651e802
b890922
a640e26
0be4262
950e8f4
c6010bd
32077e2
8ca4e0b
c3a56fc
d043a1a
567639b
6a66066
289acc3
c0e10b3
c438d5a
0b0d9d4
9765a13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
😆
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 way a user would receive this warning even if the quantum circuit was fine, right? Could an error be raised if a user creates a mutable QNode? (E.g. hashing the circuit operations: name of the operation and wire they act on and raising an error if the latest hash differs from the stored hash for the circuit.)
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.
Yes, this is now done: instead of hashing the tape arguments, we use the hash of the circuit graph.
There are performance hits for both approaches: we need to use
set_parameters()
for the former and we need to serialize for the latter. I hope to summarize more the relative performance in a follow up comment.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.
Could only the qtape have the
_cache_execute
attribute? Since a QNode can access the attributes of aqtape
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.
Ideally, but unfortunately the
self.construct()
method wipes the previous QTape and starts with a fresh one, so this line allows the cache to persist across multiple QTapes.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.
Ah, this answers my question above! Probably a good idea to add a line comment, since Antal and I both independently had the same question
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.
If caching is on, can we avoid redundant tape constructions?
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 just had another quick try of: when caching is on, tape is only constructed first time.
This results in the tests related to mutability and classical processing to fail. For example, the cache is still being used when parameters are the same but the circuit differs. Although, I'm not sure if the problem is even deeper deeper, since isn't construction the place where the input arguments are fixed to the gate ops?
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.
Are these two properties needed? It doesn't seem that they are used anywhere. Is this to allow the user to modify the cache size dynamically on an existing QNode?
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 point! I decided to remove the setter but keep the property, just in case users want to see the current caching value.
With the setter there, I was half thinking to let users dynamically set the cache. This is fine if they set it to a bigger number than the current size, but if setting smaller we need to drop multiple parts of the existing cache. I thought this was a bit of an overcomplication for now, so just got rid of the setter.
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 would be good to document this behaviour: users could also think that once the
caching
limit is reached, then nothing happens to the cache. However, this seems to be not the case as each time there's a new execution, the very first cached result is dropped and we're adding the latest result.Overall also might be worth considering which of the two options is more beneficial.
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 point! I've added it to the
caching
part of the docstring: 5bd1d37I think the current behaviour makes the most sense, else you might end up with a cache that is very stale. Moreover, we always want to be caching the last execution since that is the most likely one to be repeated.