-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add filtering methods to schedule #2597
Add filtering methods to schedule #2597
Conversation
…ervals, instruction types, or pass custom filters. Add some tests
qiskit/pulse/schedule.py
Outdated
@@ -186,6 +188,87 @@ def flatten(self) -> 'ScheduleComponent': | |||
"""Return a new schedule which is the flattened schedule contained all `instructions`.""" | |||
return ops.flatten(self) | |||
|
|||
def filter(self, *filter_funcs: List[Callable], | |||
channels: Iterable[int] = [], | |||
instruction_types: Iterable[abc.ABCMeta] = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this type, tbh. It's not an Instruction
instance, it's an Instruction
subclass (used as a type). This is the result of, for example, type(PulseInstruction)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case it should be Iterable[Instruction]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's right, because:
isinstance(PulseInstruction, Instruction)
is False
. An instance of a PulseInstruction
is an Instruction
, but the type PulseInstruction
is not an Instruction
. I guess we can go with the output from type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, you're right. I now understand what you mean. I believe in this case the typing Type
exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter interface is looking great. The fixes I've suggested are mostly focused on input types to use the preexisting pulse classes.
qiskit/pulse/schedule.py
Outdated
@@ -186,6 +188,87 @@ def flatten(self) -> 'ScheduleComponent': | |||
"""Return a new schedule which is the flattened schedule contained all `instructions`.""" | |||
return ops.flatten(self) | |||
|
|||
def filter(self, *filter_funcs: List[Callable], | |||
channels: Iterable[int] = [], | |||
instruction_types: Iterable[abc.ABCMeta] = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case it should be Iterable[Instruction]
?
qiskit/pulse/schedule.py
Outdated
@@ -186,6 +188,87 @@ def flatten(self) -> 'ScheduleComponent': | |||
"""Return a new schedule which is the flattened schedule contained all `instructions`.""" | |||
return ops.flatten(self) | |||
|
|||
def filter(self, *filter_funcs: List[Callable], | |||
channels: Iterable[int] = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be None
. Using a list as a default argument is dangerous since it is mutable and initialized on function definition. This can lead to undesired behaviour with consecutive function calls.
@@ -95,7 +97,7 @@ def channels(self) -> Tuple[Channel]: | |||
return self.timeslots.channels | |||
|
|||
@property | |||
def _children(self) -> Tuple[ScheduleComponent]: | |||
def _children(self) -> Tuple[Tuple[int, ScheduleComponent], ...]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this :)
qiskit/pulse/schedule.py
Outdated
return isinstance(time_inst[1], tuple(types)) | ||
return instruction_filter | ||
|
||
def only_intervals(intervals: Iterable[Tuple[int, int]]) -> Callable: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also accept an Interval
instance? Input tuples could be then converted to intervals for checks.
qiskit/pulse/schedule.py
Outdated
Args: | ||
filter_funcs: A list of Callables which follow the above format | ||
""" | ||
valid_subschedules = reduce(lambda sched, f: list(filter(f, sched)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to argue this too much and will let this go through, but for the sake of completeness, I'm linking Guido's thoughts on reduce and filter. Personally, I understand how they can be confusing but don't mind lambda/filter/reduce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to change it. It was a fun exercise but it's a good call out -- it doesn't help with readability
qiskit/pulse/schedule.py
Outdated
instruction_types: For example, [PulseInstruction, AcquireInstruction] | ||
intervals: Time intervals to keep, e.g. [(0, 5), (6, 10)] | ||
""" | ||
def only_channels(channels: Set[int]) -> Callable: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is desired for the channels
to be iterable containing type Channel
. That way we can specify say only DriveChannel(0)
or AcquireChannel(1)
.
qiskit/pulse/schedule.py
Outdated
@@ -186,6 +188,87 @@ def flatten(self) -> 'ScheduleComponent': | |||
"""Return a new schedule which is the flattened schedule contained all `instructions`.""" | |||
return ops.flatten(self) | |||
|
|||
def filter(self, *filter_funcs: List[Callable], | |||
channels: Iterable[int] = [], | |||
instruction_types: Iterable[abc.ABCMeta] = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding an instructions
and commands
default filter that matches specific instances of an Instruction/Command
? For example, it might be useful in testing to find out what times a given command happens at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels like a slightly different, easier task -- this method might be a bit heavy handed for it. By that I mean, if you wanted to do that, you wouldn't think to write a filter and pass your schedule through it, and then loop through the results to get the times, you'd just write a very simple for loop and collect all times as you go through. The straight python way seems easier, for the writer and reader, and is probably faster.
Perhaps I need more practical examples of the use case? I'm happy to add a separate get_times
search method to do this (since time/existence would seem to be the only new info you could get once you have an exact instruction instance).
…t-terra into pulse-filtering-methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Lauren, almost there!
391bad3
to
e87ed5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add filtering methods to schedule. Can filter over channels, over intervals, over time ranges, instruction types, or pass custom filters. Add some tests * Update changelog
Summary
Closes #2443
Details and comments
New
Schedule
methodfilter(self, *filter_funcs, channels, instruction_types, intervals) -> 'Schedule':
.Method returns a new Schedule with only the instructions which pass though the provided filters. Custom filters may be provided. If a list of channel indices is provided, only the instructions that involve that channel (and maybe also others) will be included in the new schedule. Similarly for instruction_types, only the instructions which are instances of the provided types will be included. For intervals, instructions will be retained if their timeslots are all wholly contained within any of the given intervals.