Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pulse Compiler - Scheduling pass #11981
Pulse Compiler - Scheduling pass #11981
Changes from all commits
7cd1396
a35abed
592aba6
8c83957
b670356
edf3749
52158f6
1f0e13c
8c19fbf
4b91f55
21dc703
13393b4
c828ab5
f363271
50ec916
8052bfa
5fe6c78
e185741
01803ed
0a45805
5bc3df4
251a18d
94b4ab8
0d71eeb
e696bde
a629f18
5c6b021
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 curious why this needs to be a defaultdict? Since the scheduler logic naively assumes all previous nodes are scheduled, it's safer to raise a key error (or particular compiler error) instead of silently substituting t0=0.
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
_time_table
attribute is adefaultdict
because we use it forinitial_time
,duration
etc.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.
Can we use internal
_time_table
for them and typecast to dict when the property returns?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.
If you meant to do
return dict(self._time_table)
- that would return a different object and make_time_table
it hard to mutate the base line dictionary. Did you have something else in mind?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's good point. Let's return same object as is.
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.
Can we provide this functionality as a pass? Seems like no existing pass requires flatten. I'm okey with visualizing a graph with nested (unflatten) sequences, if this is only for visualization purpose. Some target might require flatten if it doesn't have notion of context, so having the flatten pass still makes sense to me.
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.
For the visualization purpose we can add a dedicated visualization function to the
qiskit.visualization
module, and call the flatten pass internally depending on the argument, e.g.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 am not sure if this is something that should be a pass or a built-in method. I tend towards the latter because that's an operation which would probably be a stand alone - you want to visualize, or you want to flatten before sending to the backend. I don't think the user\developer should bother with a pass manager for something like that.
In other words - if the most likely scenario is a pass manager with only one pass, I think that's a good indication that it shouldn't be a pass.
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.
In case you submit the IR to backend, you know target code and whether flatten is required or not. For example, if you output Schedule like payload flatten is required, but something like qe-compiler Dialect or OpenPulse code, they have context and we can keep nested context. In this sense, a pass for flatten seems reasonable to me.
In my thoughts, only Transform pass can touch the graph structure. IR should NOT modify the structure of IR itself. Having this method obscures the responsibility, i.e. SRP
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 makes sense as well. I will split flatten out in a separate PR. (It wasn't introduced in this PR - only its tests)