Fix bugs in to_chain_structure #168
Conversation
- Copy the circuit correctly in to_chain_structure - Correctly handle measurement in the circuit.
| @@ -27,7 +28,8 @@ def to_chain_structure(qc, setup="linear"): | |||
| # FIXME This huge block has been here for a long time. | |||
| # It could be moved to the new compiler section and carefully | |||
| # splitted into smaller peaces. | |||
| qc_t = QubitCircuit(qc.N, qc.reverse_states) | |||
| qc_t = deepcopy(qc) | |||
There was a problem hiding this comment.
Does QubitCircuit implement its own copy support? E.g. an implementation of __copy__ or __deepcopy__. If not, I recommend adding it now and not relying on the default deepcopy which just copies all attributes and is not guaranteed to be sensible for this case.
There was a problem hiding this comment.
Thanks for the quick feedback! What would be the advantages of implementing our own copy? I feel like the implementation will just be copying a few attributes. And if a new attribute is added, the method also has to be updated every time.
There was a problem hiding this comment.
It's a bit tough to explain succinctly, because in the very simple cases the default copy will work. The issue is that it is not guaranteed to work and there are many many possible edge cases where it won't work. This is why __copy__ and __deepcopy__ exist.
If you don't explicitly implement __copy__ then someone modifying QubitCircuit might do something that causes __copy__ to do the wrong thing, and then code that calls copy would be safe.
If we implement __copy__ then we are committing to supporting copy(qc) working. I.e. we're making it part of our API, and that seems like a good thing. Without __copy__ we are making no such guarantee.
QubitCircuit maintains quite a lot of internal state and just copying that state might do the wrong thing. I wouldn't be able to tell for sure that the default copy does the right thing without checking thoroughly, and that is part of why we should write our own __copy__ -- because it would be guaranteed to do the right thing for QubitCircuit while the default would not (boh now and in the future).
I'm about 90% sure I could dig into QubitCircuit and find a problem, but whether there actually is an issue is not the point. The point is to provide certainty that copy will work and to make it clear to ourselves that we are committing to make it work.
It's entirely possible that the implementation of __copy__ might be somewhat trivial, and that's completely okay.
|
Not really very confident about my implementation. Let's discuss next week @hodgestar :) |
|
@BoxiLi Let's review and rework a bit if needed on Monday. |
|
Hi Simon, would it be ok if we just fix the error by using |
|
I'm also okay with a simpler fix if you prefer. |
|
Fix the bug in The failing test is because of qtuip-qtrl. We should use the master branch instead of the released ones for the check of qutip.master branch. |
|
I see there is also a warning for |
Yes... That is likely the docsting using LaTeX without adding an |
Fix #165