-
Notifications
You must be signed in to change notification settings - Fork 10
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
Emulator #849
Emulator #849
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #849 +/- ##
==========================================
+ Coverage 66.81% 70.12% +3.30%
==========================================
Files 55 61 +6
Lines 5970 6620 +650
==========================================
+ Hits 3989 4642 +653
+ Misses 1981 1978 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@jykhoo1987, looks like there are duplicate keys in some yaml files, e.g. |
@scarrazza thanks for pointing it out, it is now fixed in ea54511 |
Couple of trivial suggestions:
|
Hi Alessandro, thanks for the suggestions. But I don't think the current local pre-commit check can identify the problem according to my previous commits, but I could be wrong. |
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 @jykhoo1987 for working on this. I wrote a first batch of comments.
Another more general suggestion is to move the emulator related platforms defined in instruments/simulator/emulator_test
to tests/
, potentially to a new folder, eg. tests/emulators
. This way they will not be a part of the qibolab package and testing will also be more symmetric to what we do for the other drivers (see tests/dummy_qrc
).
src/qibolab/instruments/simulator/emulator_test/default_q0/ibmfakebelem_q0_runcard.yml
Outdated
Show resolved
Hide resolved
...qibolab/instruments/simulator/emulator_test/ibmfakebelem_q01/ibmfakebelem_q01_runcard_dt.yml
Outdated
Show resolved
Hide resolved
src/qibolab/instruments/simulator/emulator_test/ibmfakebelem_q01/parameters.json
Outdated
Show resolved
Hide resolved
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.
Just a quick skim. I'm missing all the main components (i.e. qutip_backend
and pulse_simulator
)
src/qibolab/instruments/simulator/emulator_test/default_q0/ibmfakebelem_q0_runcard.yml
Outdated
Show resolved
Hide resolved
src/qibolab/instruments/simulator/emulator_test/ibmfakebelem_q01/parameters.json
Outdated
Show resolved
Hide resolved
Sorry if I didn't reply @yjmaxpayne, I just missed the comment. If Notice that |
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 realized I started from the wrong end, I should go back and read the docs that you wrote, and only afterward continue reading the code.
def process_op(op_list: list, connector_list: List[str], connector: str) -> list: | ||
"""Implements connectors between operators.""" | ||
index_list = [] | ||
for i in range(connector_list.count(connector)): | ||
ind = connector_list.index(connector) | ||
index_list.append(ind + len(index_list)) | ||
connector_list.pop(ind) | ||
|
||
new_op_list = [] | ||
for i, op in enumerate(op_list): | ||
if i in index_list: | ||
next_op = op_list[i + 1] | ||
if i - 1 in index_list: | ||
new_op_list[-1] = op_connectors_dict[connector]( | ||
new_op_list[-1], next_op | ||
) | ||
else: | ||
new_op_list.append(op_connectors_dict[connector](op, next_op)) | ||
elif i - 1 in index_list: | ||
pass | ||
else: | ||
new_op_list.append(op) | ||
return new_op_list, connector_list |
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 possible, avoid the closure and place this top-level.
On the topic of documentation, there is currently a jupyter notebook in the examples folder and another emulator.rst document in the tutorial folder. @scarrazza suggested to have only either one of them. What are your preferences @stavros11 @alecandido ? |
I would suggest that either the notebook becomes part of the docs (but this would be the only notebook - so, for the time being, I'd even avoid) or |
Sorry folks, I miss clicked Hi @alecandido , do you think we just need to keep |
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.
Still need to review the notebook, qutip_backend.py
, and pulse_simulator.py
.
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 the updates. A few comments regarding the notebook (listing here because opening the raw notebook on GitHub is not very convenient):
- Usually we use
create_platform
to instantiate platforms instead of importing eachcreate
. Therefore instead of
from default_q0 import create
emulator_platform = create()
which also requires adding default_q0
to the path to work, you should be able to do something like
import os
os.environ["QIBOLAB_PLATFORM"] = # path to the runcard (can also be set beforehand)
from qibolab import create_platform
platform = create_platform("default_q0")
- It is better to call
Platform
methods, such asconnect
orsetup
, directly from thePlatform
object instead of accessing the instrument within the platform, for symmetry with other platforms. If there are issues with that, we should fix them, either in the new driver (emulator) or in thePlatform
itself (if it cannot accomodate the emulator driver for some reason). platform.start()
andplatform.stop()
no longer exist.
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.
Another batch of comments, now just on pulse_simulator.py
(yet far to finish it).
self.model_config = model_config | ||
self.sim_opts = sim_opts | ||
|
||
self.all_simulation_backends = {"Qutip": QutipSimulator} |
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.
TL;DR: assuming a single backend for a while is not a bad idea
Even this one could be just a top-level constant, and it could come from the backends
package.
However, are you planning to add other backends soon? It is true that it is correct to plan in advance, but if it's going to be a single backend for a while, there is no need to complicate the present layout for a potential future addition (since when it will come, it could be much different from how you are currently imagining 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.
As mentioned in a previous comment, the plan is to have other engines but for now I will just stick to QuTiP until when it is merged into qibolab and has all its core features implemented.
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.
Reviewed example 1 in the notebook. For the time being, I will skip comments on the others (since many comments could be similar)
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.
Another batch, I will stop here for a while.
def dec_to_basis_string(x: int, nlevels: list = [2]) -> list: | ||
"""Converts an integer to a generalized bitstring in the computation basis | ||
of the full Hilbert space. | ||
|
||
Args: | ||
x (int): The integer (decimal number) to be converted. | ||
nlevels (list): The list of nlevels to convert the decimal number to. Defaults to [2]. | ||
|
||
Returns: | ||
list: Generalized bitstring of x. | ||
""" | ||
nqubits = len(nlevels) | ||
output_list = [] | ||
y = x | ||
subdims_ = np.multiply.accumulate(nlevels) | ||
subdims = (subdims_[-1] / subdims_).astype(int) | ||
|
||
for sub_dim in subdims: | ||
coeff = np.divmod(y, sub_dim)[0] | ||
output_list.append(coeff) | ||
y -= coeff * sub_dim | ||
|
||
return output_list | ||
|
||
|
||
def make_comp_basis( | ||
qubit_list: List[Union[int, str]], qid_nlevels_map: dict[Union[int, str], int] | ||
) -> np.ndarray: | ||
"""Generates the computational basis states of the Hilbert space. | ||
|
||
Args: | ||
qubit_list (list): List of target qubit indices to generate the local Hilbert space of the qubits that respects the order given by qubit_list. | ||
qid_nlevels_map (dict): Dictionary mapping the qubit IDs given in qubit_list to their respective Hilbert space dimensions. | ||
|
||
Returns: | ||
`np.ndarray`: The list of computation basis states of the local Hilbert space in a numpy array. | ||
""" | ||
nqubits = len(qubit_list) | ||
|
||
qid_list = [str(qubit) for qubit in qubit_list] | ||
nlevels = [qid_nlevels_map[qid] for qid in qid_list] | ||
comp_basis_list = [] | ||
comp_dim = np.prod(nlevels) | ||
for ind in range(comp_dim): | ||
comp_basis_list.append(dec_to_basis_string(ind, nlevels=nlevels)) | ||
|
||
return np.array(comp_basis_list) |
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 would consider replacing these two functions with just a cartesian product of [np.arange(n) for n in nlevels]
.
On how to implement either a simple or an efficient cartesian product, I'd redirect you to the literature:
https://stackoverflow.com/q/11144513
(using dtype=np.uint8
should be sufficient, and it would even be more space efficient than the string for n > 10
- though it's pretty much irrelevant)
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 take it back: if you can avoid generating the whole Cartesian product, it would be only better: just use integers in [0, np.prod(nlevels))
range as the state labels, and then use dec_to_basis_string
(possibly vectorized) only when strictly needed.
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.
Just to clarify, so no change is needed?
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.
Well, I still believe this function, and especially the called one could be quite optimized (by using more NumPy calls and fewer or no loops), but maybe it is not strictly needed.
The better solution would be not to make use of this function at all, and instead just rely on integer to computational basis conversion only in the situations where it is strictly needed (better if using always one or the other representation).
However, to do this I have to review all the places where this function is used...
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.
ok in that case I'll leave this as it is now and KIV for now.
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.
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 apologize, I assumed it was a global acronym xD
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.
Simplified make_comp_basis
in d8b2ebd.
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 function is now optimal. I knew it could have been simpler with NumPy, but I was unaware of np.indices
function: it's exactly what you needed.
If you don't mind, I would still keep this conversation open for another while. There is only one option that could be even better: don't generate it all. But I'm not sure that's even true.
I believe you're using that more or less as a function n: (index := [i0, i1, ...])
. If you always need to compute all the values of this function, what you're doing is already optimal, because you're essentially precomputing it in the optimal way, and then looking up (you should avoid only if you're exhausting memory, it's a space-time tradeoff).
However, if you can avoid using all the values, because you can evaluate on-demand (following end-user requests), then we could save some evaluations.
However, it's already more than fine as it is, I just want to give it a second thought later on.
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 @jykhoo1987.
I think some qibocal protocols cannot be executed because there is something going on with the sweeper. Perhaps the readout_pulse instance is modified by the sweeper?
Co-authored-by: Andrea Pasquale <andreapasquale97@gmail.com>
…ue/False to true/false, simple code clean up in test platforms and pulse_simulator.py
Co-authored-by: Andrea Pasquale <andreapasquale97@gmail.com>
Co-authored-by: Andrea Pasquale <andreapasquale97@gmail.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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 @jykhoo1987 for addressing all the comments.
I was wondering about this point that I previously mentioned:
I think some qibocal protocols cannot be executed because there is something going on with the sweeper. Perhaps the readout_pulse instance is modified by the sweeper?
As a starting version we could consider merging this PR. For the point above I suggest to open an issue.
So I think what is left is mainly with the demo notebook. We can always merge it first and then raise issue/make a separate PR to convert it to an .rst file. As for readout pulses, there are two points on this:
|
@@ -0,0 +1,95 @@ | |||
{ |
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 believe this file is not needed.
@@ -0,0 +1,284 @@ | |||
{ |
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.
Same for this file.
"topology": [], | ||
"nqubits": 1, | ||
"ncouplers": 0, | ||
"qubits_list": ["0"], | ||
"couplers_list": [], | ||
"nlevels_q": [3], | ||
"nlevels_c": [], | ||
"readout_error": {"0": [0.01, 0.02]}, | ||
"drive_freq": {"0": 5.090167234445013}, | ||
"T1": {"0": 88578.48970762537}, | ||
"T2": {"0": 106797.94866226273}, | ||
"lo_freq": {"0": 5.090167234445013}, | ||
"rabi_freq": {"0": 0.333}, | ||
"anharmonicity": {"0": -0.3361230051821652}, | ||
"coupling_strength": {} |
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.
This seems like a runcard inside the runcard, as several keys (nqubits
, ncouplers
, topology
, etc.) are repeated. Most likely we can avoid this duplication, however I am fine with postponing this to a later iteration.
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 we went through this before, but I believe we settled on this approach at least for the time being.
- `rabi_length_sequences` | ||
- `t1_sequences` | ||
- `t2_sequences` | ||
- `allxy` | ||
- `standard_rb` |
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.
Based on this list, it seems to me that no routines involving sweepers work. I understand that sweepers involving duration/delays are not very well defined now (this is true for other drivers too), so I would postpone these fixes to 0.2. However, I would expect at least the rabi (amplitude) routine to work, since this involves only a sweeper in the drive amplitude (it doesn't affect the readout) and it is using discrimination (not integration) which is also supported.
I am fine with merging this without sweeper support, however I would prefer raising a NotImplementedError
under the sweep
method, instead of having not working code.
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.
Rabi (amplitude) routine works I believe, in fact the original runcards were recalibrated by @andrea-pasquale and @sorewachigauyo via qibocal
.
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 you please provide a qibocal runcard with this routine and parameters that work? When I tried yesterday it looked more like noise. Also, according to @andrea-pasquale only the five routines listed above work, which don’t have sweepers.
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 remember if we run rabi_amplitude
, for sure rabi_length_sequences
works. I tried this morning again with rabi_amplitude
but the output is not reasonable. Did you manage to run rabi_amplitude
@sorewachigauyo ?
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 that case I was mistaken. Yes I think rabi_amplitude
in general has an issue, not so much related to sweep
but I believe there was some missing factor issue that I am not sure we managed to resolve with @sorewachigauyo in the end. It is a subtlety with how the rabi amplitude enters into the model, not so much a qibocal
related issue. Nonetheless, maybe the easiest way to temporary address this is to add the NotImplementedError
as @stavros11 suggested and add it once it is resolved. However, the sweep
method does work in general.
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 output of the sweep
method in the emulator has exactly the same format as the dummy
platform, since that's the only reference I have. If that is different from what qibocal
requires, do let me know how it should be.
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.
Nope, Qibocal should make use of dummy. To check the results shapes, there are dedicated tests
https://github.com/qiboteam/qibolab/blob/main/tests/test_result_shapes.py
you can try to run them with the emulator as backend (later on, for the emulator we could even run these tests in the CI).
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.
However, the Qibocal output seems to follow the last sweep execution, so it looks like a problem with how the results are being accumulated.
Is this specifically for rabi_amplitude
? Just wondering if this is a problem specific to rabi_amplitude
or other routines in qibocal
that require sweep
.
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 found the problem. It is related to how samples are reshaped before passing it to the results object. It didn't matter for dummy since outputs are random numbers. This issue should be fixed in eefa732, @stavros11 @andrea-pasquale @sorewachigauyo please try to run rabi_amplitude
or any other sweeper routine in qibocal
again and let me know if this problem is still not resolved.
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 @jykhoo1987, I just run rabi_amplitude
and it seems to work as expected http://login.qrccluster.com:9000/AxNpUTVTT1-Hjw1MNYOtdQ==/
I will test also other protocols using qibocal.
for more information, see https://pre-commit.ci
@stavros11 @alecandido @jykhoo1987 in 465fd1d I updated the list with protocols that I was able to run using Qibocal. I found it curious that I was not able to run |
I'd wait for @stavros11 approval, and then we can merge. |
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 @jykhoo1987 for fixing the issue and @andrea-pasquale for checking the routines. I also ran some of those and they seem to work. It should be fine to merge.
I found it curious that I was not able to run
t1
due to aKeyError
related to theReadoutPulse
. We can open an issue about this and merge as it is in my opinion.
I suspect this is because the pulses that are swept are mutated in
setattr(pulse, param_name, sweeper_op(value, base)) |
and the updated serial, with a different start time than the original, is used in the returned result. I guess the issue only appears in T1 because it is the only one (among the supported routines) that is sweeping something on the readout pulse.
I agree with opening the issue for now. I was having a look out of curiosity, if I manage to find a quick fix I will open another PR. Anyway, this will need to be addressed in 0.2 because pulses will be immutable.
Let me also have a look at this, I suspect it might just be a small bug/typo and should be a quick fix. |
So I found the problem and it was because I copied a part from In any case, please test again |
Thanks @jykhoo1987, now |
Thanks everyone, for the implementation and reviews! I'm merging. Let's welcome to the emulator in Qibolab :D |
Checklist:
Build hacks (@alecandido additions):
macos-13
because of the reason described in Emulator #849 (comment)