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.

Sign up for GitHub

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

Jump to bottom

#2262 | Acquire commands on a single qubit #3574

Merged
merged 33 commits into from
Jan 28, 2020
Merged

#2262 | Acquire commands on a single qubit #3574

merged 33 commits into from
Jan 28, 2020

Conversation

IceKhan13
Copy link
Member

@IceKhan13 IceKhan13 commented Dec 9, 2019 •
edited
Loading

Summary

Make Acquire commands being applied to single qubit

Implements #2262

Details and comments

The pulse specification has acquire commands being applied to be multiple qubits. Within the terra API acquires should only be applied to a single qubit and then at assembly time the correct acquire command should be output by combining acquires of the same time and duration. This will make pulse programming more consistent and easier to reason about as all operations in pulse would then be for single channels.

@taalexander taalexander added the type: enhancement It's working, but needs polishing label Dec 9, 2019
@lcapelluto lcapelluto self-assigned this Dec 10, 2019
@taalexander
Copy link
Contributor

This is looking like it's coming along nicely 👍 .

@IceKhan13
Copy link
Member Author

IceKhan13 commented Dec 14, 2019 •
edited
Loading

Yeah, working on fixing timeslots merge error brought by changing acquire commands, let's see how it will go :)

@IceKhan13 IceKhan13 changed the title [WIP] #2262 | Acquire commands on s single qubit #2262 | Acquire commands on a single qubit Dec 18, 2019
@IceKhan13
Copy link
Member Author

@taalexander @lcapelluto
🤔
I'm not sure, but it looks like it's ready for review 😄

@lcapelluto
Copy link
Contributor

@IceKhan13 This is great! If we didn't need a deprecation period, I'd say it's pretty much done. Unfortunately, we should have a period where multiple qubit acquisitions work as well as single acquires. Some of the work you've done will have to be rolled back for a while, to accept both styles, with a DeprecationWarning when there's more than 1 qubit supplied to Acquire

lcapelluto
lcapelluto suggested changes Jan 6, 2020
View reviewed changes
Copy link
Contributor

@lcapelluto lcapelluto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Deprecate old multiqubit acquire style (leave / add one test for this)

@lcapelluto
Copy link
Contributor

@IceKhan13 With this change, I think a method in qiskit.pulse.utils analogous to circuit.quantumcircuit.measure_all would be really helpful. Could we add it to this PR? It should only be a few lines, but we could be correct and open a separate issue/PR instead.

@IceKhan13 IceKhan13 mentioned this pull request Jan 8, 2020
Closed
@lcapelluto
Copy link
Contributor

lcapelluto commented Jan 8, 2020 •
edited
Loading

#3700 I made the issue -- thinking maybe utils isn't the best place for the method. Anyway, I think we can keep it separate from this PR. Up to you if you want to pursue this task or not. We really appreciate the contributions you make :)

taalexander
taalexander previously requested changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

@IceKhan13 this looking awesome! Just a couple more small suggestions and then it looks good to go to me!

qiskit/qobj/converters/pulse_instruction.py Outdated Show resolved Hide resolved
qiskit/pulse/commands/acquire.py Outdated Show resolved Hide resolved
qiskit/pulse/commands/acquire.py Outdated Show resolved Hide resolved
qiskit/pulse/commands/acquire.py Outdated Show resolved Hide resolved
qiskit/qobj/converters/pulse_instruction.py Show resolved Hide resolved
@IceKhan13 IceKhan13 requested a review from taalexander January 25, 2020 00:17
@IceKhan13
Copy link
Member Author

Updated release notes + fixed as suggested

lcapelluto
lcapelluto previously approved these changes Jan 27, 2020
View reviewed changes
taalexander
taalexander previously approved these changes Jan 27, 2020
View reviewed changes
Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

LGTM!

@lcapelluto lcapelluto dismissed stale reviews from taalexander and themself via 2f1a2c3 January 27, 2020 18:40
lcapelluto
lcapelluto reviewed Jan 27, 2020
View reviewed changes
Copy link
Contributor

@lcapelluto lcapelluto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Going to remove the 'issues:' section of the release note myself, to respond to Kevin's feedback, so we can hopefully get this merged today.

Update: not permitted to do this. We will wait on @IceKhan13 :)

releasenotes/notes/acquire-single-channel-ea83cef8d991f945.yaml Outdated Show resolved Hide resolved
releasenotes/notes/acquire-single-channel-ea83cef8d991f945.yaml Outdated Show resolved Hide resolved
releasenotes/notes/acquire-single-channel-ea83cef8d991f945.yaml Outdated Show resolved Hide resolved
releasenotes/notes/acquire-single-channel-ea83cef8d991f945.yaml Outdated Show resolved Hide resolved
@IceKhan13
Copy link
Member Author

@lcapelluto sorry, done :)

@mergify mergify bot merged commit fbc3f00 into Qiskit:master Jan 28, 2020
SooluThomas added a commit to SooluThomas/qiskit-terra that referenced this pull request Jan 29, 2020
@SooluThomas
Merge pull request #3 from SooluThomas/test/measure_after_new_merge
c6508e1 
Update code with changes included from Qiskit#3574
@SooluThomas SooluThomas added the Changelog: API Change Include in the "Changed" section of the changelog label Feb 4, 2020
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
@IceKhan13 @lcapelluto @mergify
Qiskit#2262 | Acquire commands on a single qubit (Qiskit#3574)
f158e2b 
* Acquire: AcquireInstruction change to sigle qbit, mem_slot, reg_slot

* Acquire: change usage of AcquireInstruction

* Tests: partial test fixing

* Assembler: tweak scheduler assembler to laverage new acquire + tests fixes

* Scheduler: fix scheduler to use acquire on single qubit + tests fix

* Style: fix lint errors

* Acquire: remove unecessary properties

* Style: fix lint errors

* Acuqire: back compatibility on acquire multiple qubits

* Acuqire: fix linter

* Acquire: refactor

* Acquire: fix implicit acquires; todo: revisit _validate_meas_map method

* Acquire: pylint fix

* Acquire: minor fixes

* Acquire: fix add implicits acquires function

* Acquire: instruction properties fix

* Acquire: remove test for validating meas map

* Linter: fix errors

* Acquire: style fix

* Acquire: remove deprecation in acquires property + fix positional arguments

* Acquire: back and forward compatibility

* Acquire: remove deprecation warning (hm, I thought I removed it before... weird ;) )

* Acquire: grammar fixes + split schedule acquire test

* Acquire: add release note

* Acquire: release notes remove issues

Co-authored-by: Lauren Capelluto <laurencapelluto@gmail.com>
@1ucian0 1ucian0 added the mod: pulse Related to the Pulse module label Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdk kdk kdk left review comments

@lcapelluto lcapelluto lcapelluto approved these changes

@ajavadia ajavadia ajavadia approved these changes

@taalexander taalexander taalexander approved these changes

@eggerdj eggerdj Awaiting requested review from eggerdj

@diego-plan9 diego-plan9 Awaiting requested review from diego-plan9

@1ucian0 1ucian0 Awaiting requested review from 1ucian0

@mtreinish mtreinish Awaiting requested review from mtreinish

@jyu00 jyu00 Awaiting requested review from jyu00

@nkanazawa1989 nkanazawa1989 Awaiting requested review from nkanazawa1989

Assignees

@lcapelluto lcapelluto

Labels
Changelog: API Change Include in the "Changed" section of the changelog mod: pulse Related to the Pulse module type: enhancement It's working, but needs polishing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@IceKhan13 @taalexander @lcapelluto @kdk @ajavadia @1ucian0 @SooluThomas