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

schedule() considers instruction map #10988

Merged
merged 16 commits into from
Oct 19, 2023
Merged

Conversation

MozammilQ
Copy link
Contributor

Summary

In issue #10837 , I discovered that the schedule of qiskit.compiler works fine for BackendV1 but doesn't work for BackendV2. See this comment for details.

Details and comments

Details:
On inspection I discovered that measure function in macros.py requires backend, It uses backend to decide which of the two private methods _measure_v1 or _measure_v2 to call.
So, I decided to pass in this backend from the user to measure of macros.py
I did this as follows:
1>> I changed the definition of schedule_circuit in qiskit/qiskit/scheduler/schedule_circuit.py and added backend as Optional here.
2>> I used this backend and passed it to as_soon_as_possible and as_late_as_possible of qiskit/qiskit/scheduler/methods/basic.py, I also changed the definition of these two methods as_soon_as_possible and as_late_as_possible.
3>> I passed this backend to lower_gates by changing the definition of lower_gates in qiskit/qiskit/scheduler/lowering.py
4>> I also changed the definition of the nested function get_measure_schedule of lowering.py.
5>> Finally I passed in the backend to measure of macros.py

I have made backend in the definition of these changed methods Optional because I do not want the tests to fail, complaining the methods require backend as an additional argument.
I have reformatted the files that I changed and made sure that all the tests have passed! :) 

Comments:
qiskit/qiskit/compile/scheduler.py in the definition of schedule one cannot omit the backend because, how the inst_map is processed depends on it.Furthermore, there is no need to ask for inst_map and meas_map separately if you already have the backend with you!So, backend should not be Optional here! I think inst_map, meas_map and dt are redundant here, please correct me if I am wrong :)

Final result after issue fixed:
fix_issue_10837

…easure function of macros.py in qiskit/qiskit/pulse
@MozammilQ MozammilQ requested review from eggerdj, wshanks and a team as code owners October 6, 2023 18:41
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Oct 6, 2023
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core
  • @nkanazawa1989

@MozammilQ
Copy link
Contributor Author

sir, please see if my efforts a good enough :), please let me know if any additional changes are required :)

@mtreinish mtreinish added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: pulse Related to the Pulse module labels Oct 6, 2023
@mtreinish mtreinish modified the milestones: 0.25.3, 0.45.0 Oct 6, 2023
@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Oct 6, 2023
@nkanazawa1989
Copy link
Contributor

@to24toro Could you please review the PR first? I'll merge if this looks good.

@to24toro
Copy link
Contributor

Thank you @MozammilQ .
It almost is good for me and will help solving the issue of scheduling with backendV2.

I have two additional requests for improvement.

  • Add an argument of backend for the sequence in
    def sequence(scheduled_circuit: QuantumCircuit, schedule_config: ScheduleConfig) -> Schedule:
    and lower_gates in this sequence.
  • Include explanations for added arguments to pass tests lint.

qiskit/scheduler/lowering.py Outdated Show resolved Hide resolved
MozammilQ and others added 3 commits October 11, 2023 18:26
Since, get_measure_schedule is an innner function , so backend from lower_gates will be available to the inner function, so backend argument in the inner function is redundant :)

Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>
…ument from the function call of get_measure_schedule
@MozammilQ
Copy link
Contributor Author

Thank you @to24toro for your suggestions.

I have done some improvements.

* Add an argument of `backend` for the `sequence` in https://github.com/Qiskit/qiskit/blob/947e175edca77900cf1b0b266c55361bca0a7add/qiskit/scheduler/sequence.py#L28

DONE

   and `lower_gates` in this `sequence`. 

DONE

* Include explanations for added arguments to pass tests lint.

I do not understand what I am supposed to do here, anyways I have added relevant docstring in the changed methods which I forgot to add, am I supposed to do anything else? adding explanations for added arguments in which file? and I have run all tests with tox and all tests have passed.

@to24toro
Copy link
Contributor

Thank you @MozammilQ . I will confirm it.
Anyway, you need to pass all the test to merge it (see :https://dev.azure.com/qiskit-ci/qiskit/_build/results?buildId=50336&view=logs&jobId=86d13450-b8b4-5d9a-22f1-9f7e71062a71&j=86d13450-b8b4-5d9a-22f1-9f7e71062a71&t=a7f62dd9-9d78-5db3-3068-aed07887bd76). Could you please run tox -eblack and tox -elint in your PC and add the modification, referring to it?

@MozammilQ
Copy link
Contributor Author

@to24toro , :)
Aaah!! Now, I got it! thanks for the kind words, you explained it very well.
I ran tox -eblack and tox -elint, and did the required modifications in the last commit. Is it good this time? :)
just for the information I have run tox -eblack/tox -elint on fedora 38.

@to24toro
Copy link
Contributor

to24toro commented Oct 13, 2023

Thank you @MozammilQ .
I think we are almost done. The left thing is to write test codes for backendV2 because the existing test codes related with your modification use backendV1.

Especially, it's better to add the backendV2 version of

class TestBasicSchedule(QiskitTestCase):
. These tests don't include handling the case that backend is backendV2.
However, this requires some knowledges and it might be tough.

qiskit/scheduler/methods/basic.py Outdated Show resolved Hide resolved
qiskit/scheduler/methods/basic.py Outdated Show resolved Hide resolved
qiskit/scheduler/schedule_circuit.py Outdated Show resolved Hide resolved
qiskit/scheduler/sequence.py Outdated Show resolved Hide resolved
@MozammilQ
Copy link
Contributor Author

@TsafrirA , I have added many tests that uses BackendV2, there are a few tests that I am yet to add in, I am in talks with @to24toro , regarding that :)

MozammilQ and others added 5 commits October 15, 2023 15:31
Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>
Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>
Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>
Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>
@MozammilQ
Copy link
Contributor Author

Thank you @MozammilQ . I think we are almost done. The left thing is to write test codes for backendV2 because the existing test codes related with your modification use backendV1.

Especially, it's better to add the backendV2 version of

class TestBasicSchedule(QiskitTestCase):

. These tests don't include handling the case that backend is backendV2.
However, this requires some knowledges and it might be tough.

@to24toro , please see if this is ok :)

Copy link
Contributor

@to24toro to24toro left a comment

Choose a reason for hiding this comment

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

Thank you @MozammilQ for adding tests. Your comments about the pulse durations is very clear and helps me understand it. I suggested the minor changes.

I would like you to add the release note about the bugfix (see: https://github.com/Qiskit/qiskit/blob/main/CONTRIBUTING.md#release-notes)

Comment on lines 509 to 516
def test_unavailable_defaults(self):
"""Test backend with unavailable defaults."""
qr = QuantumRegister(1)
qc = QuantumCircuit(qr)
backend = FakeBackend(None)
backend.defaults = backend.configuration
self.assertRaises(QiskitError, lambda: schedule(qc, backend))

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_unavailable_defaults(self):
"""Test backend with unavailable defaults."""
qr = QuantumRegister(1)
qc = QuantumCircuit(qr)
backend = FakeBackend(None)
backend.defaults = backend.configuration
self.assertRaises(QiskitError, lambda: schedule(qc, backend))

This test is not related with the backend version and you can remove it.

Comment on lines 518 to 526
"""Test ALAP scheduling."""
# ┌────┐ ░ ┌────┐ ┌─┐
# q0_0: ┤ √X ├─░─┤ √X ├────────■──┤M├───
# ├────┤ ░ ├────┤┌────┐┌─┴─┐└╥┘┌─┐
# q0_1: ┤ √X ├─░─┤ √X ├┤ √X ├┤ X ├─╫─┤M├
# └────┘ ░ └────┘└────┘└───┘ ║ └╥┘
# c0: 2/═══════════════════════════╩══╩═
# 0 1

Copy link
Contributor

@to24toro to24toro Oct 16, 2023

Choose a reason for hiding this comment

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

I don't understand that you change the number of gates from backendV1?
Isn't it enough to replace U2 gates with sx gates?

@1ucian0 1ucian0 changed the title fixed_issue_10837 schedule() considers instruction map Oct 16, 2023
Comment on lines 2 to 26
prelude: >
Added support for BackendV2 in schedule of qiskit.compiler.scheduler.
Test methods in test/python/scheduler/test_basic_scheduler.py have been replicated with backend
being BackenV2.
features:
- |
Adds support for BackendV2 in schedule of qiskit.compiler.scheduler.
Example usage::
#import a fake backend which is a sub-class of BackendV2.
from qiskit.providers.fake_provider import FakePerth
from qiskit.compiler.scheduler import schedule

from qiskit.circuit import QuantumCircuit
qc=QuantumCircuit(1, 1)
qc.x(0)
qc.measure(0,0)
sched=schedule(circuits=qc, backend=FakePerth())


- |
Adds tests with backend being a sub-class of BackendV2.
In test file:: test/python/scheduler/test_basic_scheduler.py
All the test methods that used BackendV1 has been replicated to use BackendV2.


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prelude: >
Added support for BackendV2 in schedule of qiskit.compiler.scheduler.
Test methods in test/python/scheduler/test_basic_scheduler.py have been replicated with backend
being BackenV2.
features:
- |
Adds support for BackendV2 in schedule of qiskit.compiler.scheduler.
Example usage::
#import a fake backend which is a sub-class of BackendV2.
from qiskit.providers.fake_provider import FakePerth
from qiskit.compiler.scheduler import schedule
from qiskit.circuit import QuantumCircuit
qc=QuantumCircuit(1, 1)
qc.x(0)
qc.measure(0,0)
sched=schedule(circuits=qc, backend=FakePerth())
- |
Adds tests with backend being a sub-class of BackendV2.
In test file:: test/python/scheduler/test_basic_scheduler.py
All the test methods that used BackendV1 has been replicated to use BackendV2.

Thank you for adding the release notes and sorry for my poor explanation. It would be sufficient to just add an item for "fixes". Also, please describe what was fixed in the fixes section. It would be good to briefly mention what was fixed, such as enabling scheduling using backendV2.

@MozammilQ
Copy link
Contributor Author

@to24toro , is everything good now? :)

Copy link
Contributor

@to24toro to24toro left a comment

Choose a reason for hiding this comment

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

Thank you @MozammilQ . This Bugfix definitely helps users.

def lower_gates(
circuit: QuantumCircuit,
schedule_config: ScheduleConfig,
backend: Optional[Union[BackendV1, BackendV2]] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little nitpicky, but all these unions could be replaced with Backend.

nkanazawa1989
nkanazawa1989 previously approved these changes Oct 18, 2023
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @MozammilQ for contribution and @to24toro for review. I just added suggestion of the release note cleanup. You can push commit button if you like it. I'll approve the PR as is so that we can safely add this PR to merge queue before the release date.

…kit.compiler.scheduler-a0366d3397c73af0.yaml

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , does this look good now? :)

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @MozammilQ this looks good to me. I'll add this to merge queue.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6568856424

  • 17 of 17 (100.0%) changed or added relevant lines in 5 files are covered.
  • 15 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.009%) to 86.954%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 90.91%
crates/qasm2/src/parse.rs 12 97.13%
Totals Coverage Status
Change from base Build 6568317014: -0.009%
Covered Lines: 73738
Relevant Lines: 84801

💛 - Coveralls

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Oct 19, 2023
Merged via the queue into Qiskit:main with commit 4603e05 Oct 19, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: pulse Related to the Pulse module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants