Do not trust on Instruction.label as an instruction identifier

[1ucian0]

In #175 (comment), the example trusts on the instruction label as an instruction identifier. In general, it is not good idea to use strings for that.

In Terra, we suggest using the instruction instance for such a thing (and its str(id(instance)) for when the string identifier is needed, like in the QObj dump situation). This would reduce the coupling with Terra, since we could remove/change label at any point (we are going to keep it for know because the Qiskit/qiskit#2286 use case).

[chriseclectic]

@1ucian0 The simulator uses serialised qobj as input, so the only identifier are the strings that end up there. That PRs example is outdated since it was made by directly modifying qobj before any support was in place in Terra, so the "correct" way to do it now is like in this tutorial, (though one can always hack the qobj directly if they know what they are doing).

Keep in mind the instruction label is an important requirement for customized noise simulations required for research. In particular this makes it much easier to add multiple different types of identity operations to simulator idle qubit relaxation errors which we will be building features around soon. If you plan on changing it in Terra please let us know so we can update, and consider our input for why it was added in the current iteration so that a suitable alternative that fits our needs can be added in its place.

[1ucian0]

@1ucian0 The simulator uses serialised qobj as input, so the only identifier are the strings that end up there.

Correct. Our argument is that the string should be internally generated (for example str(id(instance))) and not exposed to userland.

That PRs example is outdated since it was made by directly modifying qobj before any support was in place in Terra, so the "correct" way to do it now is like in this tutorial, (though one can always hack the qobj directly if they know what they are doing).

In that example, the suggestion would be:

iswap = cx_circ.unitary(iswap_op, [qr[0], qr[1]])
...
noise_model.add_all_qubit_quantum_error(iswap_error, iswap)

Keep in mind the instruction label is an important requirement for customized noise simulations required for research. In particular this makes it much easier to add multiple different types of identity operations to simulator idle qubit relaxation errors which we will be building features around soon. If you plan on changing it in Terra please let us know so we can update, and consider our input for why it was added in the current iteration so that a suitable alternative that fits our needs can be added in its place.

Totally agree. The goal is to work together better, while minimizing the interdependent elements. This issue is part of that conversation.

[chriseclectic]

@1ucian0 can we close this one for now?

[1ucian0]

Yes!

…

On Tue, Jul 2, 2019, 12:53 PM Christopher J. Wood ***@***.***> wrote: @1ucian0 <https://github.com/1ucian0> can we close this one for now? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#207?email_source=notifications&email_token=AAF3FZPARGBDWGTSYKMCBK3P5OBY3A5CNFSM4HLSZHTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZB43AY#issuecomment-507760003>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF3FZP655ZFNEFYN2YYD4DP5OBY3ANCNFSM4HLSZHTA> .