pasqal-io / Pulser

Library for pulse-level/analog control of neutral atom devices. Emulator with QuTiP.
Apache License 2.0
159 stars 57 forks source link

Buggy implementation of get_qubit_weight_map #629

Closed lvignoli closed 5 months ago

lvignoli commented 5 months ago

On pulser 0.16.1, I experienced ill-defined DMM maps.

(Specs: MacBookPro 2021 M1, python 3.10.2, pulser 0.16.1)

I have tracked down the issue to the ways the weight map is built:

https://github.com/pasqal-io/Pulser/blob/80cf64078e612aa4daaeca0a662f0b181c5b5700/pulser-core/pulser/register/weight_maps.py#L74-L88

While matches is expected to be a single element list, this is not checked. Because there is an equality condition on floating point numbers, it may be the empty list. When it is the empty list, the weight assigned to the qubit in the weight map is 0.0.

Relaxing the decimal precision specified with COORD_PRECISION to say, 3, recovers the correct DMM maps. Changing the order of the qubits in the register definition recovers a properly defined DMM (!!).

Here is a minimal example to reproduce the issue. Since the issue comes from comparisons of floats, it may not be reproducible on other devices and architectures.

Code ```python import dataclasses import numpy as np import pulser from pulser.devices._devices import AnalogDevice from pulser.register.special_layouts import TriangularLatticeLayout _dmm = pulser.channels.dmm.DMM( clock_period=4, min_duration=16, max_duration=2**26, mod_bandwidth=8, bottom_detuning=-2 * np.pi * 20, # detuning between 0 and -20 MHz total_bottom_detuning=-2 * np.pi * 2000, # total detuning ) device = dataclasses.replace( AnalogDevice, dmm_objects=(_dmm,), max_sequence_duration=10_000, ) tri_layout = TriangularLatticeLayout(n_traps=100, spacing=5) def define_register_with_detuning_weights(sites, labels, weights): positions = np.array([tri_layout.traps_dict[i] for i in sites]) reg = pulser.Register.from_coordinates(positions, labels=labels) det_map = reg.define_detuning_map(dict(zip(labels, weights))) qubit_weight_map = det_map.get_qubit_weight_map(reg.qubits) # Checks that the actual DMM weight is (almost) equal to the provided one. for qubit_id, expected_weight in zip(labels, weights): try: np.testing.assert_almost_equal(qubit_weight_map[qubit_id], expected_weight) except AssertionError as e: msg = f"qubit {qubit_id}: {e}" print(msg) def main(): print("Test 1") sites = np.array([31, 53, 39, 62, 43, 49, 42, 37, 48, 44, 55, 50]) labels = np.array(["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l"]) weights = np.array([0.0, 0.0, 0.0, 0.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0]) define_register_with_detuning_weights(sites, labels, weights) print() print("Test 2") # The first 4 items of each lists are put at the end. # These are the 0 weights of the DMM map. sites = np.array([43, 49, 42, 37, 48, 44, 55, 50, 31, 53, 39, 62]) labels = np.array(["e", "f", "g", "h", "i", "j", "k", "l", "a", "b", "c", "d"]) weights = np.array([1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 0.0, 0.0, 0.0, 0.0]) define_register_with_detuning_weights(sites, labels, weights) if __name__ == "__main__": main() ``` Output: ```text Test 1 qubit e: Arrays are not almost equal to 7 decimals ACTUAL: 0.0 DESIRED: 1.0 qubit f: Arrays are not almost equal to 7 decimals ACTUAL: 0.0 DESIRED: 1.0 Test 2 ```
HGSilveri commented 5 months ago

Hi @lvignoli , thanks for the detailed bug report! Upon further inspection, I believe your code is doing something unwanted.

When you define your register from your layout, your are doing:

    positions = np.array([tri_layout.traps_dict[i] for i in sites])

    reg = pulser.Register.from_coordinates(positions, labels=labels)

There are two issues with this form:

  1. The register won't be able to recognize which layout it came from.
  2. Register.from_coordinates has center=True by default, so your coordinates are being modified to center your register around the origin.

At least on my setup, I find that setting center=False is enough to make the issues disappear. Regardless, creating a register from a layout should always be done through:

reg = tri_layout.define_register(*sites, qubit_ids=labels)

If your issues persist after this correction, let me know!

lvignoli commented 5 months ago

Thank @HGSilveri for your quick feedback.

It's true that I used the register layout in an unorthodox way, merely as a proxy for a coordinates of a triangular lattice.

Does this mean that defining a DMM map on a register that does not originates from a layout is undefined behavior?

HGSilveri commented 5 months ago

Does this mean that defining a DMM map on a register that does not originates from a layout is undefined behavior?

It's not undefined per se, it's fine for emulators since they never require an underlying layout so you can just define the detuning map directly from the register. However, what you were doing here is defining a layout and a detuning map from it, and then separately a register. This increases your chances of your qubit coordinates not aligning with the traps, giving you a weight map of mostly zero weights (as you had here).

When you have a layout, it should be the unique source truth for your coordinates, so it is best practice to define both the detuning map and the register from the same layout.

Also, it is recommended to always use RegisterLayout.define_register() whenever defining a register from a layout because only in this way is the register aware of the layout it came from (which is relevant information for QPU execution).

lvignoli commented 5 months ago

However, what you were doing here is defining a layout and a detuning map from it, and then separately a register.

I am precisely not doing that, am I?

 reg = pulser.Register.from_coordinates(positions, labels=labels)
 _ = reg.define_detuning_map(dict(zip(labels, weights)))

The register is defined without any knowledge of the layout, but so is the detuning map: we pass a mapping of qubit IDs to floats.

lvignoli commented 5 months ago

My understanding of the API is that one specifies the qubit IDs, not the traps IDs. And that pulser would take care of mapping correctly the qubit-wise user intent into a correct trap-wise detuning pattern, should there be any notion of traps in the sequence.

lvignoli commented 5 months ago

The register centering part confuses me as well, since it happens before I am specifying any detuning map, and the detuning map is specified by qubit IDs. So as a user this should not be a concern to me, shouldn't it?

HGSilveri commented 5 months ago

@lvignoli You're totally right, I was fooled by the fact setting center=False solved the issue and jumped to a conclusion without carefully looking at the code, my bad.

The register centering part confuses me as well, since it happens before I am specifying any detuning map, and the detuning map is specified by qubit IDs. So as a user this should not be a concern to me, shouldn't it?

Indeed, it shouldn't be a concern to you. I think the reason it affected the test code outcomes is because it introduces changes to the coordinates that make them be incorrectly considered different from their rounded counterparts in get_qubit_weight_map().

I have the changes to fix this pretty much ready, I'll open a PR soon and ping you for review. Sorry again for the confusion on my part!

lvignoli commented 5 months ago

Thanks a lot Henrique for the swift reaction! Having a look right now 😀