Bugfix cloud noise model callable ops #485
Conversation
| @@ -537,7 +537,7 @@ def apply_errormap_stencil(self, stencil, evotype, state_space, target_labels=No | |||
| # `None` in list signals a non-present direction => skip these terms | |||
| sslbls_list = list(filter(lambda x: x is not None, sslbls_list)) | |||
| for sslbls in sslbls_list: | |||
| op_to_embed = local_errormap if (sslbls is None or state_space.is_entire_space(sslbls)) \ | |||
There was a problem hiding this comment.
It is not immediately obvious what bug would have been caused by the is_entire_space call. I'm sure it's fine, but could you explain what this fixes?
There was a problem hiding this comment.
Currently, these two fixes come from Aidan and were edge cases he ran into while doing some CloudNoiseModel simulations. I understand the other fix, but forget what necessitated this fix. @AidanGauthier do you remember why we needed to do this?
@coreyostrove btw, I'll probably move this to a draft while updating the issue description. I want to fix #441 at the same time.
There was a problem hiding this comment.
I talked to Aidan and here's the edge case this caused: local_errorgen comes from the stencil, so it has generic stencil state space labels. However, the is_entire_space call only checks after the sslbls have been mapped on line 481. In the case the mapped state space fully matches, then the still-stencil errorgen is passed through, causing downstream sslbl mismatch errors.
EmbeddedOps presumably fix this because it is already doing sslbl mapping, but it's not the most efficient thing to do in this case. @coreyostrove, unless you have another suggestion, I'll probably expand this line into an if/elif/else and handle the is_entire_space in its own block where we copy the op with the correct sslbls instead of relying on EmbeddedOp to do so.
There was a problem hiding this comment.
Sounds perfect, thanks to you and @AidanGauthier for tracking this down and explaining!
When completed, this PR will address the following issues: