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

Label and name gate should be merged and QasmQobjInstruction take name as identifier #2393

Closed
1ucian0 opened this issue May 10, 2019 · 7 comments

Comments

@1ucian0
Copy link
Member

1ucian0 commented May 10, 2019

In PR #2173 we introduced the notion of "label" for gates. I hope I could make a compelling argument in Qiskit/qiskit-aer#207 that "label" should not be used as identifier. In the same spirit, I think "name" should not be used as identifier in Instruction.assemble (the type should be used).

We do also agree that a display-as argument in conviant for #2286 . For this purpose, I think a name/label (just one of them) is useful.

For assemble purposes instruction = QasmQobjInstruction(name=self.qobj_name) and each gate should set a class attribute qobj_name.

class Reset(Instruction):
    """Qubit reset."""

    qobj_name = 'reset'

    def __init__(self, name='reset'):
        """Create new reset instruction."""
        super().__init__(name, 1, 0, [])
@1ucian0
Copy link
Member Author

1ucian0 commented May 10, 2019

The plan would be:

@jaygambetta
Copy link
Member

I would rather not couple (even with names) the qobj and the circuits.

@1ucian0
Copy link
Member Author

1ucian0 commented May 13, 2019

You think QasmQobjInstruction should extract the qboj-name from the type of the gate? Currently, instructions and gates have an assemble method. If we want to remove every qobj/circuit couple, the responsibility of assembling should be on the qobj-side, right?

@1ucian0
Copy link
Member Author

1ucian0 commented Jul 4, 2019

From #2395 (comment), we are moving towards a class attribute name for the gate names as a way to join the type and the name of the gates. I'm removing the discussion label.

@ajavadia
Copy link
Member

ajavadia commented Jul 5, 2019

@taalexander seemed to suggest this approach will not be good for pulse

@taalexander
Copy link
Contributor

The issue for pulse is the way pulse's are represented in the Qobj. Each pulse in a pulse instruction references a unique pulse in the pulse_library by its name. However in the pulse module we do not have a pulse library, we simply have instances of SamplePulse which may be instantiated with a pulse and a name (optional if not provided will automatically be assigned).

As pulse instance creation is a much more common pattern than basis gate creation in circuits it would be a very inconvenient and confusing pattern to have to derive a new class for each new pulse with a different name. In my mind creating a SamplePulse with a given set of samples is more akin to creating an instance of a Gate with a set of arguments (such as the angles for u3).

If we were set on this model we could provide a constructor function that would automatically derive and name a new class for each new set of pulse samples, but this seems clunky.

@jakelishman
Copy link
Member

I'm going to close this as stale now - the name/label split has largely been resolved to be a purely visual thing (label is used by the drawers), and even that is somewhat on its way out. The concerns about Qobj I think are long since stale as well.

Please feel free to re-open if there's more to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants