Repository: QCHackers/tqec (python)

Issue:
Define a clear standard for coordinate order in `stim` exports
**Is your feature request related to a problem? Please describe.**
The current code make extensive use of several data-classes with explicit `x` and `y` attributes to avoid having to ask ourselves the question "should `x` coordinate be in the first position of this tuple or the second?". 

The only place where we still have that question is when we manipulate `stim` data-structures, that explicitly have no standard on coordinate order (see for examples the [`DETECTOR` instruction documentation](https://github.com/quantumlib/Stim/blob/main/doc/gates.md#DETECTOR): `There is no required convention for which coordinate is which.`)

We should have a `tqec` standard on coordinate order for `stim` instructions, that we enforce on all `tqec`-generated `stim` data-structures.

**Describe the solution you'd like**
Maybe a `StimInstructionCoordinates` data-structure with explicit `x` and `y` (and `z`, or `t`, ...?) attributes and a `to_stim_coords` method fixing the ordering. That would allow us to easily change the ordering if we find that our initial choice was bad.


Pull request:
Adding `StimCoordinates` 
Fixes #359 by defining coordinate order through the `StimCoordinates` class. Code in `tqec.circuit.detectors` does not use this because we try to keep the module independent of the outside `tqec` module.

Patch:
diff --git a/src/tqec/circuit/coordinates.py b/src/tqec/circuit/coordinates.py
new file mode 100644
index 00000000..eae5cbbc
--- /dev/null
+++ b/src/tqec/circuit/coordinates.py
@@ -0,0 +1,47 @@
+"""Implements a wrapper to standardise stim coordinate system across the code base."""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+
+
+@dataclass(frozen=True)
+class StimCoordinates:
+    """Wrapper around a tuple of coordinates.
+
+    This class should be used whenever a `stim.CircuitInstruction` that expects
+    arguments that are interpreted by stim as coordinates is built. This is the
+    case of the `DETECTOR` instruction for example.
+    """
+
+    _ABS_TOL = 1e-10
+
+    x: float
+    y: float
+    t: float | None = None
+
+    def to_stim_coordinates(self) -> tuple[float, ...]:
+        if self.t is not None:
+            return (self.x, self.y, self.t)
+        return (self.x, self.y)
+
+    def __str__(self) -> str:
+        return "(" + ",".join(f"{c:.2f}" for c in self.to_stim_coordinates()) + ")"
+
+    def offset_spatially_by(self, x: float, y: float) -> StimCoordinates:
+        return StimCoordinates(self.x + x, self.y + y, self.t)
+
+    def __eq__(self, value: object) -> bool:
+        return (
+            isinstance(value, StimCoordinates)
+            and abs(self.x - value.x) < StimCoordinates._ABS_TOL
+            and abs(self.y - value.y) < StimCoordinates._ABS_TOL
+            and (
+                (self.t is None and value.t is None)
+                or (
+                    self.t is not None
+                    and value.t is not None
+                    and abs(self.t - value.t) < StimCoordinates._ABS_TOL
+                )
+            )
+        )
diff --git a/src/tqec/circuit/qubit.py b/src/tqec/circuit/qubit.py
index ad37c540..a3fa4b57 100644
--- a/src/tqec/circuit/qubit.py
+++ b/src/tqec/circuit/qubit.py
@@ -12,6 +12,7 @@ from collections import defaultdict
 
 import stim
 
+from tqec.circuit.coordinates import StimCoordinates
 from tqec.position import Displacement, Position2D
 
 
@@ -33,9 +34,10 @@ class GridQubit:
     def to_qubit_coords_instruction(self, index: int) -> stim.CircuitInstruction:
         """Return the `QUBIT_COORDS` `stim.CircuitInstruction` needed to define
         `self` in a `stim.Circuit`."""
-        # TODO: check ordering here.
         return stim.CircuitInstruction(
-            "QUBIT_COORDS", [index], [float(self.x), float(self.y)]
+            "QUBIT_COORDS",
+            [index],
+            StimCoordinates(self.x, self.y).to_stim_coordinates(),
         )
 
     def __add__(self, other: GridQubit | Position2D | Displacement) -> GridQubit:
diff --git a/src/tqec/compile/detectors/compute.py b/src/tqec/compile/detectors/compute.py
index 9fe17649..ddf7cca3 100644
--- a/src/tqec/compile/detectors/compute.py
+++ b/src/tqec/compile/detectors/compute.py
@@ -1,5 +1,6 @@
 import stim
 
+from tqec.circuit.coordinates import StimCoordinates
 from tqec.circuit.detectors.flow import build_flows_from_fragments
 from tqec.circuit.detectors.fragment import Fragment
 from tqec.circuit.detectors.match import (
@@ -68,7 +69,8 @@ def _matched_detectors_to_detectors(
                     f"but got {measurement.qubit}."
                 )
             measurements.append(measurement)
-        ret.append(Detector(frozenset(measurements), d.coords))
+        x, y, t = d.coords
+        ret.append(Detector(frozenset(measurements), StimCoordinates(x, y, t)))
     return frozenset(ret)
 
 
@@ -248,12 +250,10 @@ def compute_detectors_for_constant_template_and_fixed_radius(
                 # coordinates of detectors are derived from the qubits, which are
                 # already in the `GridQubit` convention. So we only need to swap the
                 # coordinates of the plaquette origin.
-                coordinates = (
-                    d.coordinates[0] + plaquette_origin.y,
-                    d.coordinates[1] + plaquette_origin.x,
-                    *d.coordinates[2:],
-                )
                 detectors_by_measurements[offset_measurements] = Detector(
-                    offset_measurements, coordinates
+                    offset_measurements,
+                    d.coordinates.offset_spatially_by(
+                        plaquette_origin.x, plaquette_origin.y
+                    ),
                 )
     return frozenset(detectors_by_measurements.values())
diff --git a/src/tqec/compile/detectors/detector.py b/src/tqec/compile/detectors/detector.py
index 8714018d..89e94af0 100644
--- a/src/tqec/compile/detectors/detector.py
+++ b/src/tqec/compile/detectors/detector.py
@@ -7,6 +7,7 @@ from dataclasses import dataclass
 import numpy
 import stim
 
+from tqec.circuit.coordinates import StimCoordinates
 from tqec.circuit.measurement import Measurement
 from tqec.circuit.measurement_map import MeasurementRecordsMap
 from tqec.exceptions import TQECException
@@ -17,17 +18,11 @@ class Detector:
     """Represent a detector as a set of measurements and optional coordinates."""
 
     measurements: frozenset[Measurement]
-    coordinates: tuple[float, ...]
+    coordinates: StimCoordinates
 
     def __post_init__(self) -> None:
         if not self.measurements:
             raise TQECException("Trying to create a detector without any measurement.")
-        if len(self.coordinates) < 2:
-            raise TQECException(
-                "Coordinates should at least provide 2 spatial dimensions. "
-                f"The provided  coordinates {self.coordinates} only contain "
-                f"{len(self.coordinates)} dimensions."
-            )
 
     def __hash__(self) -> int:
         return hash(self.measurements)
@@ -36,13 +31,12 @@ class Detector:
         return (
             isinstance(rhs, Detector)
             and self.measurements == rhs.measurements
-            and numpy.allclose(self.coordinates, rhs.coordinates)
+            and self.coordinates == rhs.coordinates
         )
 
     def __str__(self) -> str:
-        coordinates_str = "(" + ",".join(f"{c:.2f}" for c in self.coordinates) + ")"
         measurements_str = "{" + ",".join(map(str, self.measurements)) + "}"
-        return f"D{coordinates_str}{measurements_str}"
+        return f"D{self.coordinates}{measurements_str}"
 
     def to_instruction(
         self, measurement_records_map: MeasurementRecordsMap
@@ -79,7 +73,7 @@ class Detector:
                 )
             )
         return stim.CircuitInstruction(
-            "DETECTOR", measurement_records, self.coordinates
+            "DETECTOR", measurement_records, self.coordinates.to_stim_coordinates()
         )
 
     def offset_spatially_by(self, x: int, y: int) -> Detector:
@@ -96,5 +90,5 @@ class Detector:
 
         return Detector(
             frozenset(m.offset_spatially_by(x, y) for m in self.measurements),
-            (self.coordinates[0] + x, self.coordinates[1] + y, *self.coordinates[2:]),
+            self.coordinates.offset_spatially_by(x, y),
         )


Test patch:
diff --git a/src/tqec/circuit/coordinates_test.py b/src/tqec/circuit/coordinates_test.py
new file mode 100644
index 00000000..c0b92522
--- /dev/null
+++ b/src/tqec/circuit/coordinates_test.py
@@ -0,0 +1,26 @@
+from tqec.circuit.coordinates import StimCoordinates
+
+
+def test_creation() -> None:
+    StimCoordinates(0, 0, 0)
+    StimCoordinates(1, 0, -4)
+    StimCoordinates(1.3, 422.92348567, 0.2)
+
+
+def test_getters() -> None:
+    zeros = StimCoordinates(0, 0, 0)
+    assert zeros.x == 0
+    assert zeros.y == 0
+    assert zeros.t == 0
+
+    coords = StimCoordinates(-1, 34, 12)
+    assert coords.x == -1
+    assert coords.y == 34
+    assert coords.t == 12
+
+
+def test_offset_spatially_by() -> None:
+    coords = StimCoordinates(0, 0, 0).offset_spatially_by(34, 7)
+    assert coords.x == 34
+    assert coords.y == 7
+    assert coords.t == 0
diff --git a/src/tqec/compile/detectors/detector_test.py b/src/tqec/compile/detectors/detector_test.py
index a519ef80..8a47cad0 100644
--- a/src/tqec/compile/detectors/detector_test.py
+++ b/src/tqec/compile/detectors/detector_test.py
@@ -1,6 +1,7 @@
 import pytest
 import stim
 
+from tqec.circuit.coordinates import StimCoordinates
 from tqec.circuit.measurement import Measurement
 from tqec.circuit.measurement_map import MeasurementRecordsMap
 from tqec.circuit.qubit import GridQubit
@@ -22,29 +23,19 @@ def test_detector_creation(measurement: Measurement) -> None:
     with pytest.raises(
         TQECException, match="^Trying to create a detector without any measurement.$"
     ):
-        Detector(frozenset(), tuple())
-    with pytest.raises(
-        TQECException,
-        match="^Coordinates should at least provide 2 spatial dimensions.",
-    ):
-        Detector(frozenset([measurement]), tuple())
-    with pytest.raises(
-        TQECException,
-        match="^Coordinates should at least provide 2 spatial dimensions.",
-    ):
-        Detector(frozenset([measurement]), (1,))
+        Detector(frozenset(), StimCoordinates(0, 0, 0))
 
-    Detector(frozenset([measurement]), (0, 3))
+    Detector(frozenset([measurement]), StimCoordinates(0, 3, 0))
 
 
 def test_detector_to_instruction(
     measurement: Measurement, mrecords_map: MeasurementRecordsMap
 ) -> None:
-    detector = Detector(frozenset([measurement]), (1, 1))
+    detector = Detector(frozenset([measurement]), StimCoordinates(1, 1, 0))
     instruction = detector.to_instruction(mrecords_map)
     assert instruction.name == "DETECTOR"
     assert instruction.targets_copy() == [stim.target_rec(-1)]
-    assert instruction.gate_args_copy() == [1, 1]
+    assert instruction.gate_args_copy() == [1, 1, 0]
 
     empty_mrecords_map = MeasurementRecordsMap()
     with pytest.raises(TQECException):
@@ -52,9 +43,9 @@ def test_detector_to_instruction(
 
 
 def test_detector_offset_spatially_by(measurement: Measurement) -> None:
-    detector = Detector(frozenset([measurement]), (1, 1))
+    detector = Detector(frozenset([measurement]), StimCoordinates(1, 1, 0))
     offset_detector = detector.offset_spatially_by(45, -2)
     assert offset_detector.measurements == frozenset(
         [measurement.offset_spatially_by(45, -2)]
     )
-    assert offset_detector.coordinates == (46, -1)
+    assert offset_detector.coordinates.to_stim_coordinates() == (46, -1, 0)