-
Notifications
You must be signed in to change notification settings - Fork 26
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
[Quantum Chinese Chess] Add save_snapshot() and restore_last_snapshot() to QuantumWorld #173
Conversation
madcpf
commented
Dec 4, 2023
- Add save_snapshot() and restore_last_snapshot() to QuantumWorld
- Apply these new methods in quantum chinese chess
@@ -345,9 +346,15 @@ def test_copy(simulator, compile_to_qubits): | |||
assert board.circuit is not board2.circuit | |||
assert board.effect_history == board2.effect_history | |||
assert board.effect_history is not board2.effect_history | |||
assert board.effect_history_length == board2.effect_history_length | |||
assert board.qubit_remapping_dict_length == board2.qubit_remapping_dict_length |
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 these asserts should also have some sort a message (second argument to assert) to help callers debug issues
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 think most cases here are kind of self explanatory and would be a bit redundant to add messages. Thanks.
# and recover the effects up to the last snapshot (which was saved after the last move finished). | ||
try: | ||
world.restore_last_snapshot() | ||
except: |
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.
is a specific exception you can capture here instead of the blanked except?
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.
Right now the possible error is only ValueError. But to be future proof I think maybe we could just include all kinds of exceptions here, instead of
try:
abc
except ValueError:
return False
except:
return False
Thanks.
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 also don't think it's a good idea to catch all exceptions. Some you might not want to catch, like "KeyboardInterrupt" for instance. This is also going to swallow errors and hide them which will make debugging harder too.
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.
Thanks Doug. I'm updating the code to only catch ValueError. Other errors will be raised instead of swallowed.
# and recover the effects up to the last snapshot (which was saved after the last move finished). | ||
try: | ||
world.restore_last_snapshot() | ||
except: |
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 also don't think it's a good idea to catch all exceptions. Some you might not want to catch, like "KeyboardInterrupt" for instance. This is also going to swallow errors and hide them which will make debugging harder too.
unitary/alpha/quantum_world.py
Outdated
@@ -269,6 +290,52 @@ def undo_last_effect(self): | |||
raise IndexError("No effects to undo") | |||
self.circuit, self.post_selection = self.effect_history.pop() | |||
|
|||
def save_snapshot(self) -> None: | |||
"""Saves the current length of the effect history and qubit_remapping_dict.""" |
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 think we want a better description here. Is the idea that we save a snapshot every move?
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.
Hi Doug, yes for chess we default to save a snapshot after each player's move, so later on if they choose to undo those quantum properties could be restored. I'm adding this into the comment. (In the end it's up to the game developer to decide in what granularity they allow the player to undo.)