Emulator - Githubissues

jykhoo1987 commented 3 months ago

Checklist:

[ ] Reviewers confirm new code works as expected.
[ ] Tests are passing.
[ ] Coverage does not decrease.
[ ] Documentation is updated.

Build hacks (@alecandido additions):

[ ] the Mac runner is temporarily set to macos-13 because of the reason described in https://github.com/qiboteam/qibolab/pull/849#issuecomment-2117660675
[ ] Pylint has been forced to 3.1.0, to temporarily avoid some new lints 9104000

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.12%. Comparing base (485f416) to head (7269c6b). Report is 22 commits behind head on main.

Additional details and impacted files

```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/849/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `70.12% <100.00%> (+3.30%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

scarrazza commented 3 months ago

@jykhoo1987, looks like there are duplicate keys in some yaml files, e.g. anharmonicity in here, please have a look at the pre-commit output.

jykhoo1987 commented 3 months ago

@scarrazza thanks for pointing it out, it is now fixed in ea54511

alecandido commented 3 months ago

Couple of trivial suggestions:

set up your editor to add newlines at the end of files (it's a common convention), pre-commit fixed it in the last commit
run pre-commit install in your local repo instance (to run checks locally before committing)

yjmaxpayne commented 3 months ago

Couple of trivial suggestions:

set up your editor to add newlines at the end of files (it's a common convention), pre-commit fixed it in the last commit

run pre-commit install in your local repo instance (to run checks locally before committing)

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.

alecandido commented 3 months ago

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.

Sorry if I didn't reply @yjmaxpayne, I just missed the comment.

If pre-commit is installed, it is either failing or fixing (and failing) preventing you to commit files that are not compliant with the defined hooks (you can always bypass it with git commit -n). If it's passing locally, it should pass even in the CI (the hooks versions are exactly defined in the configuration file).

Notice that pre-commit only checks the modified files. If you want to pass a global scan locally, you should manually launch pre-commit run --all.

jykhoo1987 commented 3 months ago

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 ?

alecandido commented 3 months ago

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 emulator.rst could be an optimal solution.

yjmaxpayne commented 3 months ago

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 emulator.rst could be an optimal solution.

Sorry folks, I miss clicked close with comment.

Hi @alecandido , do you think we just need to keep doc/source/tutorials/emulator.rst? I think we can further beef up its contents based on those in Jupiter notebook. I agree that keeping things to emulator.rst will be a better option for tracking purposes.

alecandido commented 2 months ago

@jykhoo1987 Thanks for actively working on the comments, and also for reporting the hash of the commits fixing them.

Whenever you do it (i.e. you add a Fixed in xyzabcd comment) feel free to resolve the conversation! In case there is anything further to be discussed, we can always reopen them.

oschwarze commented 1 month ago

Hi @jykhoo1987, I just tried running this code, and all tests in test_emulator.py fail for me when using scipy version 1.13.0. Forcing the version of scipy to be 1.12.0 fixed this.

jykhoo1987 commented 1 month ago

Hi @jykhoo1987, I just tried running this code, and all tests in test_emulator.py fail for me when using scipy version 1.13.0. Forcing the version of scipy to be 1.12.0 fixed this.

Thanks @oschwarze for checking this. Locally I am using scipy 1.12.0 so I have no idea that it breaks with 1.13.0. But I am not sure if this is related to the tests failing here as it does not even load the platform. I don't think this process involves scipy, does it?

alecandido commented 1 month ago

Thanks @oschwarze for checking this. Locally I am using scipy 1.12.0 so I have no idea that it breaks with 1.13.0. But I am not sure if this is related to the tests failing here as it does not even load the platform. I don't think this process involves scipy, does it?

The reason why tests are failing (or were failing) in the CI is not because of dependencies. The motivation relies in missing definition/extension of QIBOLAB_PLATFORMS env var, which is needed to discover the platforms. And that's the reason why default_q0 is not found.

In particular, the CI is using the lock file, installing with Poetry (whether this is a good or a bad choice is debatable, I'm already working to do both), and thus you can read the version used in the lock file https://github.com/qiboteam/qibolab/blob/7ea0f9cbb6c149cc5138571f5764b1beccaec05b/poetry.lock#L4837-L4839

However, there is no good reason to fail with SciPy 1.13.0. If it's that giving errors (and not some other local settings), that should be fixed as well.

alecandido commented 1 month ago

Since it was opened since a while, I decided to rebase this on main, in order to avoid leaving it too much outdated.

I did it myself, because sometimes it might be annoying, if you're not aware of the changes that happened on the other side. However, the overlap of the changes in main with those in this PR was minimal, and mainly boiling down to just dependencies, so it has been pretty quick :)

alecandido commented 1 month ago

I'm not sure why this problem didn't arise before, but the current tests are failure is essentially reproduced by the following snippet:

from qutip import basis
x = basis(2, 0)
x * x.dag()

triggered by this line: https://github.com/qiboteam/qibolab/blob/d34d3582fec584672e527b236a6e407e661a3300/src/qibolab/instruments/emulator/engines/qutip_engine.py#L103

I'm trying to check if it is in any way related to the QuTiP version, or something similar. Despite the rebase, the qutip engine is fully unchanged, since it is fully orthogonal to everything that happened in main.

alecandido commented 1 month ago

Indeed, it seems that that operation is not failing in QuTiP v5.0.0, but that is incompatible in few other ways, e.g.

src/qibolab/instruments/emulator/engines/qutip_engine.py:13: in <module>
    from qutip.operators import identity as Id
E   ModuleNotFoundError: No module named 'qutip.operators'

Thus, I'm pretty user you used QuTiP <5.0.0 ...

Not sure what is happening, maybe you have a better understanding.

alecandido commented 1 month ago

Ok, much ado about nothing: I should have messed up with the QuTiP version (as expected), but between v4.7.5 and v5.0.0 there is another one, i.e. v4.7.6. It seems the emulator is only working with that one, since apparently v4.7.5 is bugged, and v5 is a major upgrade, thus it is not surprising that is incompatible.

Sorry for the intermediate spam.

alecandido commented 1 month ago

Since the CI is still failing, let me give a potential explanation in advance:

build is failing because of MacOS - there is a non-negligible chance that the only version of QuTiP compatible with the emulator has some kind of issue on that platform... (in particular, there seems to be a clash with the NumPy version, I'll further investigate and check whether I'm able to solve it), despite it's passing on the other platforms
tests are failing because of the linting: it could be due to a Pylint version upgrade; I will try to solve with a downgrade, but most likely it would be nicer to just fix the new lints

All in all, I've been able to check locally that tests are passing (despite the annoying lack of the green tick from our sometimes beloved workflows).

jykhoo1987 commented 1 month ago

Thanks for doing all the checks @alecandido! Yes I can confirm there are some issues with MacOS for qutip v4.7.6. I just installed it and things are breaking. It was working perfectly fine on my Mac with qutip v4.7.5 and scipy v1.12.0... I guess I'll just revert to that for now while I work through the rest of the comments from before.

alecandido commented 1 month ago

Thanks for doing all the checks @alecandido! Yes I can confirm there are some issues with MacOS for qutip v4.7.6. I just installed it and things are breaking. It was working perfectly fine on my Mac with qutip v4.7.5 and scipy v1.12.0... I guess I'll just revert to that for now while I work through the rest of the comments from before.

Feel free to try. I had this problem, https://github.com/qiboteam/qibolab/pull/849#issuecomment-2115968271, with v4.7.5. But if you could solve it, it'd be perfect :)

alecandido commented 1 month ago

Ok, I had a further look, and the history of QuTiP's version was a bit weird, since they released v4.7.6 after v5.0.0 (it's actually the latest release), and the project is not large to regularly maintain multiple majors...

It seems that they are having an issue with SciPy v1.13.0, and the way it's operating on sparse matrices https://github.com/qutip/qutip/pull/2383 (that I believe, in turn, to be due to some other library, as it seemed from the error messages appearing from the debugging).

In any case, I'd suggest (before merging, i.e. not necessarily immediately) to adopt QuTiP v5.0.0, since it should be the current main line of development (where we have higher chances that these bugs will keep being fixed).

P.S.: it seems the problem is still present even with v4.7.5 (declaring issues with oldest-supported-numpy) - still need to further investigate...

alecandido commented 1 month ago

Ok, @scarrazza, this is interesting: the CI is now failing because it is attempting to compile QuTiP on MacOS, since there is no package for macos-latest.

The interesting part is that QuTiP is distributing packages for MacOS x86_64, but not for MacOS ARM and... GitHub finally migrated the macos-latest runner, such that from now it will be the ARM one.

The problem now is slightly more complicated, because it's not fully in control by Poetry, since it is not happening during runtime dependency resolution, but for build time dependencies... I'm sorry @jykhoo1987, but it seems like you have been hit by the GitHub runners transition: https://github.blog/changelog/2024-04-01-macos-14-sonoma-is-generally-available-and-the-latest-macos-runner-image/

I will migrate back to macos-13 for the time being (just to see if there is any other issue with the CI), but we're about to cut out MacOS ARM users... https://github.com/qutip/qutip/issues/2405 https://github.com/qutip/qutip/issues/1740#issuecomment-1640803583 (it seems it should still be possible to use QuTiP on Apple Silicon with Conda)

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories

alecandido commented 1 month ago

Ok @jykhoo1987, now it's up to you: there is only one failing test in the docs https://github.com/qiboteam/qibolab/actions/runs/9129614227/job/25104582646?pr=849#step:12:169

Everything else seems to be passing :)

The review is not finished, but at least we're back on track with the tests ;)

alecandido commented 1 month ago

@jykhoo1987 I found this (stale) project that is doing something very similar to this emulator https://github.com/sequencing-dev/sequencing/tree/main not sure if you were also aware.

I would never rely on that (it's pretty much dead, and all the troubles we're having with QuTiP's could only be worse there), but it could be interesting to take some inspiration. In particular, they are also modelling the cavities, so maybe they have an interesting way of doing measurements :)

(I'm pointing you just for exploration, if you also believe it's interesting, we could open an issue about that - I'm not making any proposal to include in the current PR)

alecandido commented 1 month ago

Regarding the usage of .setup() and .dump() to load & store the simulator configurations, you can extend the default behavior https://github.com/qiboteam/qibolab/blob/b93ee1b029df36a9b4df48c04ce9459764957b93/src/qibolab/instruments/abstract.py#L67-L73 in the following way:

 def setup(self, **settings):
     super().setup(**settings) # or just `setup(settings["bound"])`, but it more coupled
                               # to the underlying function
     ...
     # load your parameters from the configs

 def dump(self):
     mysettings: dict
     # add your settings
     ...
     return mysettings | super().dump()

jykhoo1987 commented 1 month ago

Regarding the usage of .setup() and .dump() to load & store the simulator configurations, you can extend the default behavior

https://github.com/qiboteam/qibolab/blob/b93ee1b029df36a9b4df48c04ce9459764957b93/src/qibolab/instruments/abstract.py#L67-L73

in the following way:
 def setup(self, **settings):
     super().setup(**settings) # or just `setup(settings["bound"])`, but it more coupled
                               # to the underlying function
     ...
     # load your parameters from the configs

 def dump(self):
     mysettings: dict
     # add your settings
     ...
     return mysettings | super().dump()

I'm having some issue with the bounds in super().setup, in particular I'm getting this error... seems like Bounds only accept certain keys? I have already implemented load_instrument_settings so if I comment out this line it has no issues. If it is not clear I can push the changes.

 69 def setup(self, **settings):
 70     """Updates the pulse simulator by loading all parameters from
 71     `model_config` and `simulation_config`."""
 ---> 72     super().setup(**settings)
 73     simulation_config = settings['simulation_config']
 74     model_params = settings['model_params']

TypeError: setup() got an unexpected keyword argument 'model_params'

alecandido commented 1 month ago

I'm having some issue with the bounds in super().setup, in particular I'm getting this error... seems like Bounds only accept certain keys? I have already implemented load_instrument_settings so if I comment out this line it has no issues. If it is not clear I can push the changes.

Yes, sorry, it's not even related to the Bounds class, but just the parent method. It only accepts bounds as a parameter, and nothing else.

Just apply what I added as a comment, i.e. super().setup(settings["bounds"]). Then, add the trivial bounds to the runcards as "bounds": [1, 1, 1] (this means that each experiment will be run on its own, which makes sense for the emulator, since you have no advantage by batching, and it would only complicate the implementation).

jykhoo1987 commented 1 month ago

Here are some example reports:

Rabi length: http://login.qrccluster.com:9000/pVfH7eBHQ8CbrpWKLuxG2Q==

AllXY: http://login.qrccluster.com:9000/eeMqfX5jS3mtyEov3n11ug==

State tomography: http://login.qrccluster.com:9000/vpY6WFGuTtCbjpICdwdZUg==

Thanks @stavros11 for trying it with Qibocal and I'm glad it is already compatible! Previously I had some issues, I think it was because some required methods were missing but I guess now they are in place. Unfortunately I do not have access to those links above... I will give some more thought into the naming convention but I think I have a rough idea.

jykhoo1987 commented 1 month ago

I'm having some issue with the bounds in super().setup, in particular I'm getting this error... seems like Bounds only accept certain keys? I have already implemented load_instrument_settings so if I comment out this line it has no issues. If it is not clear I can push the changes.

Yes, sorry, it's not even related to the Bounds class, but just the parent method. It only accepts bounds as a parameter, and nothing else.

Just apply what I added as a comment, i.e. super().setup(settings["bounds"]). Then, add the trivial bounds to the runcards as "bounds": [1, 1, 1] (this means that each experiment will be run on its own, which makes sense for the emulator, since you have no advantage by batching, and it would only complicate the implementation).

Turns out "bounds" is a dict not a list. Added in 92a914109cec75e8267bf3a9512508f1b448ed0f

jykhoo1987 commented 1 month ago

@stavros11 @alecandido So I think everything is in order now and all tests are passing. The only thing remaining is a review of the completely reworked notebook and once that is done, I can convert it to emulator.rst, add it to documentation and remove the notebook.

alecandido commented 1 month ago

@stavros11 @alecandido So I think everything is in order now and all tests are passing. The only thing remaining is a review of the completely reworked notebook and once that is done, I can convert it to emulator.rst, add it to documentation and remove the notebook.

Thanks @jykhoo1987, I just asked again @stavros11 and myself for a review, and I will focus on the user interface.

It may be interesting to wait, if we can collect some further feedback by the other two reviewers listed (@andrea-pasquale and @rodolfocarobene), since they will be closer to final (but much experienced) users, possibly further refining the output of @stavros11 about the usage in calibration routines (unfortunately, I'm much less qualified for that, so I just trust @stavros11 response).

jykhoo1987 commented 1 month ago

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: 1) There is an option instant_measurement in the instrument settings under simulation_config. You can set that to false and the simulation will run the full duration of the readout pulse. 2) As mentioned previously, there are currently no models in place for the readout pulses. So nothing happens, other than T1 and T2 decay if simulate_dissipation is set to true.

andrea-pasquale commented 2 weeks ago

@stavros11 @alecandido @jykhoo1987 in https://github.com/qiboteam/qibolab/pull/849/commits/465fd1d4a5e549c01f7b07ce9143ee2098a233eb 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 t1 due to a KeyError related to the ReadoutPulse. We can open an issue about this and merge as it is in my opinion.

alecandido commented 2 weeks ago

I'd wait for @stavros11 approval, and then we can merge.

jykhoo1987 commented 2 weeks ago

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 a KeyError related to the ReadoutPulse. 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

https://github.com/qiboteam/qibolab/blob/465fd1d4a5e549c01f7b07ce9143ee2098a233eb/src/qibolab/instruments/emulator/pulse_simulator.py#L323

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.

jykhoo1987 commented 2 weeks ago

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 icarusqfpga.py incompletely. I have added back the missing part in ec915c82b7bca8f458e7a31110e9a5905f7bdc82. These are lines 256 to 264 in pulse_simulator.py. I am not sure if there is a need to "Reset pulse values back to original values", but as I am making the emulator to be compatible first with icarusq, I decided to follow as much as possible the implementation there. Perhaps @sorewachigauyo can comment on this. If it is not necessary, we can delete these lines and it should work as well.

In any case, please test again t1 protocol, it should work now.

andrea-pasquale commented 2 weeks ago

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 icarusqfpga.py incompletely. I have added back the missing part in ec915c8. These are lines 256 to 264 in pulse_simulator.py. I am not sure if there is a need to "Reset pulse values back to original values", but as I am making the emulator to be compatible first with icarusq, I decided to follow as much as possible the implementation there. Perhaps @sorewachigauyo can comment on this. If it is not necessary, we can delete these lines and it should work as well.

In any case, please test again t1 protocol, it should work now.

Thanks @jykhoo1987, now t1 and t2 are working as expected with sweepers. Also t2 echo is now available :)

alecandido commented 2 weeks ago

Thanks everyone, for the implementation and reviews!

I'm merging. Let's welcome to the emulator in Qibolab :D

qiboteam / qibolab

Emulator #849

Codecov Report