qiskit icon indicating copy to clipboard operation
qiskit copied to clipboard

Published 20 hours ago • verified publisher icon Qiskit

Add support for multi-qubit-gates to CSPLayout

Open Silrem opened this issue 3 years ago • 13 comments

Summary

Fixes #7155. Added support for multi-qubit-gates to CSPLayout. CSPLayout now assumes for every 3+-qubit-gate we need a full connection between all involved qubits.

Details and comments

I've implemented the version suggested in the issue, assuming all qubits involved in the gate, need a full connection. As this is my first issue and I'm not too familiar with the library yet, I'm wondering about a few things:

  • Should I add a test case for this fix?
  • Do we need to take into account direction? Should we require a bidirectional full connection?
  • This approach does improve CSVLayout, but is the assumption actually fair? For Toffoli it holds, but can we say every 3+-qubit-gate does require a full connection?
  • [x] Possibly add testcase
  • [x] Add release notes
  • [ ] Check how/if we need to consider direction
  • [ ] Check assumption

Silrem avatar Feb 05 '22 10:02 Silrem

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 05 '22 10:02 CLAassistant

Hey guys, what's the right way for me to get some feedback on this?

Silrem avatar Feb 20 '22 10:02 Silrem

Removing the [WIP] is the usual way to signal that :-)

1ucian0 avatar Feb 20 '22 12:02 1ucian0

Pull Request Test Coverage Report for Build 2362442011

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 84.375%
Totals Coverage Status
Change from base Build 2349198842: 0.001%
Covered Lines: 54541
Relevant Lines: 64641
💛 - Coveralls

coveralls avatar Feb 20 '22 13:02 coveralls

very well :D

Silrem avatar Feb 20 '22 14:02 Silrem

Hi @Silrem thank-you for your contribution and apologies for the slow response. Yes please do add a test case for this and a bugfix release note. You can check out the Contributing Guidelines for details on this 😄

javabster avatar Apr 07 '22 18:04 javabster

Hi @javabster, thanks for your guidance. I've now added a test case and release notes.

Silrem avatar Apr 23 '22 18:04 Silrem

Hey @Silrem it looks like you've got a linting error, you can run tox -eblack to fix this

javabster avatar May 02 '22 15:05 javabster

blacking in 096a4a00d14296ab0336f1002ee8636b843f0800

1ucian0 avatar May 21 '22 08:05 1ucian0

Do we need to take into account direction? Should we require a bidirectional full connection?

that's a complicated one. I think is better not to do it for now.

Take a look to @javabster comment about the release note.

1ucian0 avatar May 31 '22 13:05 1ucian0

@javabster thanks for your hint. I've learned some new syntax there. committed as you suggested.

Silrem avatar Jun 05 '22 14:06 Silrem

@Silrem Hey, just want to check in to see if you are still working on the PR. Can you please add the tests as suggested by Luciano?

HuangJunye avatar Aug 18 '22 10:08 HuangJunye

@HuangJunye At the moment, I'm not working on this PR anymore. Over the last months, other duties have taken over, unfortunately. I might be back in the future, but for now, feel free to let someone else complete this if they want.

Silrem avatar Sep 18 '22 07:09 Silrem

Closing this PR as contributor isn't able to complete right now. We can re-open at a later date if contributor returns 😄

javabster avatar Apr 07 '23 17:04 javabster