Make TwoQubitWeylDecomposition pickle-able #7333
Conversation
Partial fix for Qiskit#7312
Pull Request Test Coverage Report for Build 3376943375
💛 - Coveralls |
There was a problem hiding this comment.
The entire __new__ of this class terrifies me, but given that, I think this change is a sensible way to do things.
One comment about potentially testing copy.copy and copy.deepcopy as pickle-adjacent, but I'm not super bothered about them, so I've approved as well in case they're not worth doing.
| def assertRoundTripPickle(self, weyl1: TwoQubitWeylDecomposition): | ||
| """Fail if loads(dumps(weyl1)) not equal to weyl1""" |
There was a problem hiding this comment.
It may also be worth adding a very similar test that copy.deepcopy and copy.copy work correctly as well/ deepcopy presumably will because it's a full pickle/unpickle by default, but copy might do something funny; I think it basically calls __getstate__, then restores it with an immediate call to __setstate__ without recursing the pickle through the state. I would guess it does the right thing and calls __getnewargs_ex__ if defined, but I don't really know, and it might be worth a test, since Qiskit uses the copy module pretty extensively.
|
|
||
| pkl = pickle.dumps(weyl1, protocol=max(4, pickle.DEFAULT_PROTOCOL)) | ||
| weyl2 = pickle.loads(pkl) | ||
| msg_base = f"weyl1:\n{weyl1}\nweyl2:\n{repr(weyl2)}" |
There was a problem hiding this comment.
I learned recently that f"{repr(x)}" can also be written as f"{x!r}" (and !s for str), which seems like a fun way to introduce a million more sigils to the strings and make them completely unreadable. When we get to Python 3.8+ only, there's an even more arcane one: f"{weyl1=}", which is equivalent to f"weyl1={weyl1!r}".
|
Seems this PR was forgotten -- any plans to merge? |
jakelishman
added
Changelog: Bugfix
Partial fix for #7312
Probably I should have used a factory function to specialize to the appropriate subclass rather than this trickery with
__new__that is only strictly required when subclassing builtins such asint, but its an easy fix to tweak__getnewargs_ex__to pass an argument to__new__so it can recognize if it is being called in unpickle context where there is no need to redo the decomposition.