Skip to content
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

Convert Instruction.name to be read-only. #7139

Closed
wants to merge 2 commits into from

Conversation

kdk
Copy link
Member

@kdk kdk commented Oct 14, 2021

Summary

Following from some discussion in #7087 , moves Instruction.name to be a getter-only @property. The hope here is to indicate to users that Instruction.name is uniquely meaningful to the transpiler in identifying the operation type and can have unexpected consequences if changed, especially, that it not be changed to conflict with the Instruction's type, e.g. in @nkanazawa1989 's example

cx = CXGate()
cx.name = "ry"

will confuse several places in terra.

Details and comments

WIP for discussion:

  • Sufficiently useful in helping users avoide above pitfall?
  • Performance impact of additional @property?
  • Look into existing usage (two in qiskit/, two more in test/)
  • Should a corresponding @name.setter be introduced with a deprecation warning?

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Oct 14, 2021

I think this is reasonable approach in view of what currently implemented in terra. Transpiler passes and quantum info module implicitly assume name is a frozen attribute, in contrast to how actually the name is implemented. This PR will fill this gap.

On the other hand, this is in opposite direction to what #7130 is trying to implement (though we are still able to update name by directly accessing to the protected member). In the instruction schedule map, the name of instruction is somewhat of flexible property and its gate kind should be defined by type or matrix rather than name (so we need to rewrite tons of existing code).

In this sense, this PR may induce further gap between views of circuit and pulse gates. We need #5885 to bridge them. Basically V2 model can define name for microinstruction so to speak (gate is tied to schedule), and we can keep the the name of circuit instruction unchanged (so this PR will make more sense after V2 model is deployed).

@@ -98,6 +98,10 @@ def __init__(self, name, num_qubits, num_clbits, params, duration=None, unit="dt

self.params = params # must be at last (other properties may be required for validation)

@property
def name(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need same update for DAGNode.

@jakelishman
Copy link
Member

This PR is rather stale now and would likely need a lot more work if we decide to go this way, so in the interests of end-of-year tidying up, I'll close it for now. Feel free to re-open it if you want to take it further from this base.

Replacing a core attribute like name with a property could potentially have negative effects on performance (since we check it a lot), so that's something we'll need to check if we take this forwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants