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

Remove schedule component #5245

Merged
merged 12 commits into from
Oct 30, 2020
Merged

Conversation

wahaj
Copy link
Contributor

@wahaj wahaj commented Oct 16, 2020

Summary

Deprecate the ScheduleComponent interface. Replace all ScheduleComponent occurrences with Union[Schedule, Instruction] in typing

Details and comments

Refer to PR #5137

Anywhere that currently accepts a ScheduleComponent should instead accept a Union[Schedule, Instruction]. The docstrings for the removed properties have been propagated to the Instruction and the Schedule classes themselves. Schedule and Instruction both now uses a 'Schedule' / 'Instruction' forwardRef of the class in the typed arguments

Fixes #5076

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.

Hello, @wahaj. I apologize for the long delay in my response, my focus was turned away from Qiskit. After a discussion with @lcapelluto, we believe the move to define a root NamedValue is preemptive since there is parallel work underway to define a universal IR for Qiskit and we do not want to interfere. Therefore, if you could remove NamedValue and have Instruction and Schedule just be root classes this would be excellent. I apologize for the wasted work, but I do believe it was on the right track.

@wahaj
Copy link
Contributor Author

wahaj commented Oct 16, 2020

Hello, @wahaj. I apologize for the long delay in my response, my focus was turned away from Qiskit. After a discussion with @lcapelluto, we believe the move to define a root NamedValue is preemptive since there is parallel work underway to define a universal IR for Qiskit and we do not want to interfere. Therefore, if you could remove NamedValue and have Instruction and Schedule just be root classes this would be excellent. I apologize for the wasted work, but I do believe it was on the right track.

Hi @taalexander, the delay led me to believe that I was to open a new PR. Anyways I'll be removing the values module. If there is anything at all that is required to do in this PR except that ?

PS : Can we remove the channels argument in the ch_start_time channel argument

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.

I understand and this falls on me, I am sorry. Please do deprecate the channels argument, we can remove it in the following release after warning for an appropriate time. I do not see any additional changes that are required.

@wahaj wahaj force-pushed the remove-schedule-component branch 3 times, most recently from 5805933 to 2f45747 Compare October 21, 2020 08:42
import copy
import itertools
import multiprocessing as mp
import sys
from typing import List, Tuple, Iterable, Union, Dict, Callable, Set, Optional

from qiskit.circuit.parameterexpression import ParameterExpression, ParameterValueType
from qiskit.pulse import instructions
Copy link
Contributor

@taalexander taalexander Oct 21, 2020

Choose a reason for hiding this comment

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

This import is unnecessary and is causing circular import/linting issues. For the type hints just use 'Instruction'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for that. Hope it passes this time around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warning, treated as error:
Cannot resolve forward reference in type annotations of "qiskit.pulse.Schedule": name 'Instruction' is not defined
___________________________________ summary ____________________________________
ERROR:   docs: commands failed

This is the error Azure build gives me. The problem is with the forward References in Schedule and Instruction. According to PEP484 forward references section, this would cause a circular dependency either way. Can you help resolve this problem?

@wahaj wahaj force-pushed the remove-schedule-component branch from 2f45747 to d0f59ad Compare October 21, 2020 14:55
wahaj added 2 commits October 21, 2020 19:59
Deprecate the ScheduleComponent class and remove it as the base class
of Schedule and Instruction. Anywhere that currently accepts a
ScheduleComponent should instead accept a Union[Schedule, Instruction].
Propagate the docstrings for the removed properties to Instruction and
the Schedule classes themselves. Use Schedule and Instruction type
forwardRef of the classes in the typed arguments

* deprecate channels argument in ch_start_time method of Instruction

Fixes Qiskit#5076
@wahaj wahaj force-pushed the remove-schedule-component branch from d0f59ad to 1fe0755 Compare October 21, 2020 15:01
@wahaj
Copy link
Contributor Author

wahaj commented Oct 22, 2020

Warning, treated as error:
Cannot resolve forward reference in type annotations of "qiskit.pulse.Schedule": name 'Instruction' is not defined
___________________________________ summary ____________________________________
ERROR:   docs: commands failed

Hi @taalexander,
This is the error Azure build gives me. The problem is with the forward References in Schedule and Instruction. According to PEP484 forward references section, this would cause a circular dependency either way. Can you help resolve this problem?

@wahaj wahaj requested a review from taalexander October 24, 2020 08:00
@taalexander
Copy link
Contributor

Warning, treated as error:
Cannot resolve forward reference in type annotations of "qiskit.pulse.Schedule": name 'Instruction' is not defined
___________________________________ summary ____________________________________
ERROR:   docs: commands failed

Hi @taalexander,
This is the error Azure build gives me. The problem is with the forward References in Schedule and Instruction. According to PEP484 forward references section, this would cause a circular dependency either way. Can you help resolve this problem?

Agreed, the circular references are a byproduct of trying to be both a data structure and have a nice DSL like user interface. I think this was a mistake, and the qiskit.pulse.builder is taking the role of the user interface to construct a Schedule. I'll see if I can figure out how to make this work for now.

@taalexander
Copy link
Contributor

I pushed a commit which should hopefully resolve these issues, lets see :)

@mergify mergify bot merged commit f0b47e2 into Qiskit:master Oct 30, 2020
@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
Labels
mod: pulse Related to the Pulse module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ScheduleComponent class
3 participants