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
Native gates directly defined by the platform #1077
Native gates directly defined by the platform #1077
Changes from 12 commits
8423633
0886c0d
0674c61
ed0f495
5190fe0
ee44f8d
ec80474
efd70aa
b433c46
266e3a7
3331404
aa92440
f8f69de
68e2cd7
2d6d8ce
2ae7fde
e1c0047
a04745a
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.
Is there a reason to start from parameters, instead of starting directly from the natives listing exposed by the platform?
https://github.com/qiboteam/qibolab/blob/bfb48cc2d9ec5e563110720889dc74acca6e604c/src/qibolab/backends.py#L64-L79
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 I understood, this function should return the native gates that have a rule in the default compiler, so it will not work if the
iSWAP
is in the platform (this was my first attempt but as explained before the result was not what I was looking for, I am not sure this is intended).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.
Wait a second: this function is used just to feed
set_compiler
, in which you're doing the followingqibocal/src/qibocal/auto/transpile.py
Lines 151 to 154 in e1c0047
To me, that means that you're trying to support rules beyond the default compiler. Including
iSWAP
, if available.I'm sure I misunderstood something in your reply...
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 wonder if
Z
should be somehow directly inferred fromRZ
by the transpiler itself...(implying that is not platform's/Qibocal's business)
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 theory yes, I am using Qibolab compiler and NativeGates convention to differentiate between the two.
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.
Fine, but is there a reason to differentiate?
I.e. with the current compiler, the
RZ
will always be implemented as virtual phase, and that will be the same for theZ
as well...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.
No, maybe @andrea-pasquale could have an explanation.
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.
Using closures is always delicate, so, I'd suggest moving it in a function on its own, at least will make it clear which are the captured variables
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.
P.S.: type hints would also help
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.
Also, usually it is better to avoid mutating objects, especially nested ones.
I could lift the suggestion by one level, and suggest creating the entire backend (here just returning the compiler, instead of modifying the backend). But let's limit to the compiler itself.
You could create an empty dictionary
rules
, and here doeventually setting the compiler out of the loops as:
(in principle, you can avoid even modifying the dictionary
rules
, by defining it immediately through comprehension - but I get this is a bit unfamiliar).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.
Properly document that this function is not pure, since it has the side effect of modifying the input
backend
(in particular, affecting its.compiler
attribute).I know this seems pedantic, but losing track of this changes may have effects later on, when they will become unexpected.
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.
My idea at the beginning was to make the
set_compiler
function in each protocol that executes circuits explicit. I decided to put it here to make my life easier but I will open an issue about it.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.
No need.
I agree with your choice, since there is nothing protocol-specific, so it should not be repeated over and over.
Everyone's life easier. Good choice :)
Even better, you could have put it here
qibocal/src/qibocal/auto/execute.py
Lines 263 to 264 in f8f69de
if it were the single place where the backend was created.
Since unfortunately it is not, instead of this:
qibocal/src/qibocal/protocols/state_tomography.py
Lines 105 to 106 in f8f69de
you could actually just make a single function, and invoke as
considering that this double functions invocation is repeat over and over.
https://github.com/search?q=repo%3Aqiboteam%2Fqibocal%20construct_backend&type=code
(in any case, this one could be an issue - but it's especially relevant for
0.2
, since we won't maintain0.1
for long)