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

no validity check on Target construction #8914

Closed
ajavadia opened this issue Oct 16, 2022 · 4 comments · Fixed by #8925
Closed

no validity check on Target construction #8914

ajavadia opened this issue Oct 16, 2022 · 4 comments · Fixed by #8925
Assignees
Labels
bug Something isn't working

Comments

@ajavadia
Copy link
Member

Environment

  • Qiskit Terra version:
  • Python version:
  • Operating system:

What is happening?

It's possible to build a Target where the Instruction does not match the number of qubits it's being added to.

How can we reproduce the issue?

For example here I add a 1-qubit gate to a two-qubit tuple.

from qiskit.transpiler import Target, InstructionProperties
from qiskit.circuit.library import RZGate
from qiskit.circuit import Parameter

θ = Parameter('θ')
rz_props = {
    (0, 1): InstructionProperties(duration=0, error=0)
}

target = Target()
target.add_instruction(RZGate(θ), rz_props, name='rz')

What should happen?

raise Error

Any suggestions?

No response

@ajavadia ajavadia added the bug Something isn't working label Oct 16, 2022
@ajavadia
Copy link
Member Author

ajavadia commented Oct 16, 2022

I also feel like this shouldn't be valid, but currently is:

target = Target(2)
target.add_instruction(CXGate())

i.e. adding an instruction to unknown qubits. It will later fail if we try to do things like target.operation_names_for_qargs((0,1))

@jakelishman
Copy link
Member

For the main issue: seems fine to me to add the checks, unless Matthew has reasons not to.

Your newest comment looks like a bug in operation_names_for_qargs to me - the construction example you gave is explicitly allowed and means "this instruction is valid on all pairs of qubits". There may need to be an extra tracker for operations that are defined over None, and the numbers of qubits they're valid for.

@ajavadia
Copy link
Member Author

Yeah I think this 2nd one is a bug, and is currently blocking #8917 by causing 2 tests to fail.

@mtreinish
Copy link
Member

So the first point is half a feature half a bug. The add_instruction() method allows you to add instructions with new qubits and extend the qubits in the target. Something like:

target = Target()
target.add_instruction(RXGate(), {(0,): None, (1,): None, (2,): None})

To make a target with 3 qubits. The idea behind that was for generating target's programmatically it was easier to just parse an instructions payload and get a valid target without having to know the number of qubits ahead of time. But I agree it's a bug that we're not validating the number of qubits matches the operation width.

The second thing is definitely a bug as we should check the number of qubits for globally defined operations and return them in operation_names_for_qargs().

@mtreinish mtreinish self-assigned this Oct 17, 2022
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Oct 17, 2022
This commit fixes the handling in the target for working with
instructions defined globally in the target. It adds handling to the
operation_names_for_qargs() and operations_for_qargs() methods to build
outputs that include globally defined operations instead of ignoring
them. Additionally the add_instruction() method is updated to validate
the input qargs to ensure that only valid qubits and qargs are allowed
to be added to the target for an instruction.

Fixes Qiskit#8914
@mergify mergify bot closed this as completed in #8925 Oct 18, 2022
mergify bot added a commit that referenced this issue Oct 18, 2022
This commit fixes the handling in the target for working with
instructions defined globally in the target. It adds handling to the
operation_names_for_qargs() and operations_for_qargs() methods to build
outputs that include globally defined operations instead of ignoring
them. Additionally the add_instruction() method is updated to validate
the input qargs to ensure that only valid qubits and qargs are allowed
to be added to the target for an instruction.

Fixes #8914

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot pushed a commit that referenced this issue Oct 18, 2022
This commit fixes the handling in the target for working with
instructions defined globally in the target. It adds handling to the
operation_names_for_qargs() and operations_for_qargs() methods to build
outputs that include globally defined operations instead of ignoring
them. Additionally the add_instruction() method is updated to validate
the input qargs to ensure that only valid qubits and qargs are allowed
to be added to the target for an instruction.

Fixes #8914

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 3a073c7)
ajavadia pushed a commit to ajavadia/qiskit that referenced this issue Oct 18, 2022
This commit fixes the handling in the target for working with
instructions defined globally in the target. It adds handling to the
operation_names_for_qargs() and operations_for_qargs() methods to build
outputs that include globally defined operations instead of ignoring
them. Additionally the add_instruction() method is updated to validate
the input qargs to ensure that only valid qubits and qargs are allowed
to be added to the target for an instruction.

Fixes Qiskit#8914

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot added a commit that referenced this issue Oct 18, 2022
This commit fixes the handling in the target for working with
instructions defined globally in the target. It adds handling to the
operation_names_for_qargs() and operations_for_qargs() methods to build
outputs that include globally defined operations instead of ignoring
them. Additionally the add_instruction() method is updated to validate
the input qargs to ensure that only valid qubits and qargs are allowed
to be added to the target for an instruction.

Fixes #8914

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 3a073c7)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants