DiCarloLab-Delft / PycQED_py3

Python3 version of PycQED using QCoDeS as backend
MIT License
67 stars 40 forks source link

Fixed issue that VNA measurements were not averaged #717

Closed MiniSean closed 1 year ago

MiniSean commented 1 year ago

Fixes #716.

Changes proposed in this pull request:

@savalless

In order for the pull request to be merged, the following conditions must be met:

Whenever possible the pull request should

Tests are not mandatory as this is generally hard to make for instruments that interact with hardware.

MiniSean commented 1 year ago

Test failing are due to CI not being able to build openql. Does not even reach pytest.

MiniSean commented 1 year ago

Update to build specific commit version of qutechopenql seems to work on CI. Now only failing on certain test-cases. (Note: qutechopenql build does take roughly 20 min for some reason..)

wvlothuizen commented 1 year ago

(Note: qutechopenql build does take roughly 20 min for some reason..)

with the default build settings some OpenQL passes are included that require a library called Eigen, which takes very long to build. It would be possible to disable that, but it might be of more use to get an official release out (how does that sound to you, @pablolh)

MiniSean commented 1 year ago

Only two more tests failing (both in test_device_objects.py): test_measure_two_qubit_randomized_benchmarking test_measure_two_qubit_allxy While performing these dummy measurements a RuntimeError pops up:

self = <openql.openql.Platform; > args = ('OpenQL_Platform', 'D:\...\PycQED_py3\pycqed\tests\dev_qubit_objs\test_output_cc\config_cc_s17_direct_iq_openql_0_10.json')

def init(self, args): r""" init(Platform self, std::string const & name, std::string const & platform_config, std::string const & compiler_config="") -> Platform init(Platform self, std::string const & name="none") -> Platform """ _openql.Platform_swiginit(self, _openql.new_Platform(args))

RuntimeError: Internal compiler error: Error in JSON definition: key 'cc' not found on path 'instructions/_fluxdance_1', actual node contents '{"decomposition":{"into":"{ cz q[10],q[8] | cz q[9],q[12] }"},"duration":0,"prototype":[]}' Stack trace (most recent call last):

4 Object "", at 0000026FC0699120, in ??

3 Object "", at 00000000000006A6, in ??

2 Object "", at 0000000000000002, in ??

1 Object "", at 0000026F00000000, in ??

0 Object "", at 00007FFB8A336725, in PyInit__openql

So there is a missing keyword for 'cc' in the generated config file. My question is:

MiniSean commented 1 year ago

Figured out how these config files are generated. Missing keys in common configurations presets:

"cc": {
    "signal": []
},

Affected files:

The keys have been put back in the config presets. However, I think it is a good idea to address the core issue here "Why does a runtime error occur because of these missing keys?"

pablolh commented 1 year ago

Hi @MiniSean I am unfamiliar with the cc backend in general but there is some docs on these keys: https://openql.readthedocs.io/en/latest/gen/reference_architectures.html#additional-instruction-data

Interestingly the intent in the code seems to be that there is a default value if they are not provided, and that shouldn't raise an error:

Settings::SignalDef Settings::findSignalDefinition(const Json &instruction, const Str &iname) const {
    SignalDef ret;

    Str instructionPath = "instructions/" + iname;
    if(!QL_JSON_EXISTS(instruction, "cc")) {
        ret.signal = {};
        ret.path = instructionPath;
    } else {
        [...]
    }
   return ret;
}
MiniSean commented 1 year ago

@pablolh Thank you for the link to the documentation. I believe this functionality is indeed working as intended, just that the openql version on my setup was not up to date. Now running from source (version 0.10.5) the "cc" key is no longer necessary.

wvlothuizen commented 1 year ago

Great you found out yourselves. Indeed the handling of key "cc" was changed, see https://github.com/DiCarloLab-Delft/OpenQL/blob/0.10.4/CHANGELOG.md. The reason for the change was to allow gate definitions without the "cc" key for instructions that are decomposed and never reach the backend anyway.

Readthedocs is rather outdated since formal releases haven't been created for quite some time. I've tried to keep the sources for the docs aligned with the changes

MiniSean commented 1 year ago

@wvlothuizen It took a while, but I managed to fix CI for python 3.7, 3.8 and 3.9. That means this can finally be merged and I can start fixing all other branches :)