Repository: rigetti/pyquil (python)

Issue:
Make the `get_attribute` and `set_attribute` methods on `DefFrame` public.
`DefFrame` supports generic attributes but there is not a direct way to access any that aren't defined as a `@property` in the public API. One example is `channel_delay`. It was added to the constructor parameters in `V4`, but doesn't have an associated property and requires going through the underlying `quil` API to access it. We should make the current `_get_attribute` and `_set_attribute` methods public so any key/value pairs can be fetched or modified.

~The `DefFrame` `channel_delay` property wasn't carried over from V3. We should add a special `@property` definition for it so it works as it used to.~


Discussion:
Historical note: it wasn't carried over because it didn't exist, right? This is a new feature of the v2 translation backend.

And simultaneously, frames changed in the Quil spec to have dynamic key-value pairs. So I don't think the right answer here is to add more properties, but rather to allow looking them up by string key, and deprecating the property-based lookups.
Ah, yeah, that is correct, `channel_delay` is new to V4. We should make the current get/set attribute methods public, and point users to it instead. 

Patch:
diff --git a/pyquil/quilbase.py b/pyquil/quilbase.py
--- a/pyquil/quilbase.py
+++ b/pyquil/quilbase.py
@@ -2750,12 +2750,12 @@ def frame(self) -> Frame:
     def frame(self, frame: Frame) -> None:
         quil_rs.FrameDefinition.identifier.__set__(self, frame)  # type: ignore[attr-defined]
 
-    def _set_attribute(self, name: str, value: Union[str, float]) -> None:
+    def set_attribute(self, name: str, value: Union[str, float]) -> None:
         updated = super().attributes
         updated.update({name: DefFrame._to_attribute_value(value)})
         quil_rs.FrameDefinition.attributes.__set__(self, updated)  # type: ignore[attr-defined]
 
-    def _get_attribute(self, name: str) -> Optional[Union[str, float]]:
+    def get_attribute(self, name: str) -> Optional[Union[str, float]]:
         value = super().attributes.get(name, None)
         if value is None:
             return None
@@ -2763,46 +2763,98 @@ def _get_attribute(self, name: str) -> Optional[Union[str, float]]:
             return value.to_string()
         return value.to_expression().to_number().real
 
+    def __getitem__(self, name: str) -> Union[str, float]:
+        if not isinstance(name, str):
+            raise TypeError("Frame attribute keys must be strings")
+        value = self.get_attribute(name)
+        if value is None:
+            raise AttributeError(f"Attribute {name} not found")
+        return value
+
+    def __setitem__(self, name: str, value: Union[str, float]) -> None:
+        if not isinstance(name, str):
+            raise TypeError("Frame attribute keys must be strings")
+        self.set_attribute(name, value)
+
     @property
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use get_attribute('DIRECTION') instead.",
+    )
     def direction(self) -> Optional[str]:
-        return self._get_attribute("DIRECTION")  # type: ignore
+        return self.get_attribute("DIRECTION")  # type: ignore
 
     @direction.setter
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use set_attribute('DIRECTION') instead.",
+    )
     def direction(self, direction: str) -> None:
-        self._set_attribute("DIRECTION", direction)
+        self.set_attribute("DIRECTION", direction)
 
     @property
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use set_attribute('INITIAL-FREQUENCY') instead.",  # noqa: E501
+    )
     def initial_frequency(self) -> Optional[float]:
-        return self._get_attribute("INITIAL-FREQUENCY")  # type: ignore
+        return self.get_attribute("INITIAL-FREQUENCY")  # type: ignore
 
     @initial_frequency.setter
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use set_attribute('INITIAL-FREQUENCY') instead.",  # noqa: E501
+    )
     def initial_frequency(self, initial_frequency: float) -> None:
-        self._set_attribute("INITIAL-FREQUENCY", initial_frequency)
+        self.set_attribute("INITIAL-FREQUENCY", initial_frequency)
 
     @property
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use get_attribute('HARDWARE-OBJECT') instead.",
+    )
     def hardware_object(self) -> Optional[str]:
-        return self._get_attribute("HARDWARE-OBJECT")  # type: ignore
+        return self.get_attribute("HARDWARE-OBJECT")  # type: ignore
 
     @hardware_object.setter
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use set_attribute('HARDWARE-OBJECT') instead.",
+    )
     def hardware_object(self, hardware_object: str) -> None:
-        self._set_attribute("HARDWARE-OBJECT", hardware_object)
+        self.set_attribute("HARDWARE-OBJECT", hardware_object)
 
     @property
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use get_attribute('SAMPLE-RATE') instead.",
+    )
     def sample_rate(self) -> Frame:
-        return self._get_attribute("SAMPLE-RATE")  # type: ignore
+        return self.get_attribute("SAMPLE-RATE")  # type: ignore
 
     @sample_rate.setter
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use set_attribute('SAMPLE-RATE') instead.",
+    )
     def sample_rate(self, sample_rate: float) -> None:
-        self._set_attribute("SAMPLE-RATE", sample_rate)
+        self.set_attribute("SAMPLE-RATE", sample_rate)
 
     @property
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use get_attribute('CENTER-FREQUENCY') instead.",
+    )
     def center_frequency(self) -> Frame:
-        return self._get_attribute("CENTER-FREQUENCY")  # type: ignore
+        return self.get_attribute("CENTER-FREQUENCY")  # type: ignore
 
     @center_frequency.setter
+    @deprecated(
+        version="4.0",
+        reason="Quil now supports generic key/value pairs in DEFFRAMEs. Use set_attribute('CENTER-FREQUENCY') instead.",
+    )
     def center_frequency(self, center_frequency: float) -> None:
-        self._set_attribute("CENTER-FREQUENCY", center_frequency)
-        self._set_attribute("CENTER-FREQUENCY", center_frequency)
+        self.set_attribute("CENTER-FREQUENCY", center_frequency)
 
     def __copy__(self) -> Self:
         return self


Test patch:
diff --git a/test/unit/test_quilbase.py b/test/unit/test_quilbase.py
--- a/test/unit/test_quilbase.py
+++ b/test/unit/test_quilbase.py
@@ -518,32 +518,48 @@ def test_frame(self, def_frame: DefFrame, frame: Frame):
         assert def_frame.frame == Frame([Qubit(123)], "new_frame")
 
     def test_direction(self, def_frame: DefFrame, direction: Optional[str]):
-        assert def_frame.direction == direction
+        assert def_frame.direction == direction is None if not direction else def_frame["DIRECTION"]
         def_frame.direction = "tx"
         assert def_frame.direction == "tx"
 
     def test_initial_frequency(self, def_frame: DefFrame, initial_frequency: Optional[float]):
-        assert def_frame.initial_frequency == initial_frequency
+        assert (
+            def_frame.initial_frequency == initial_frequency is None
+            if not initial_frequency
+            else def_frame["INITIAL-FREQUENCY"]
+        )
         def_frame.initial_frequency = 3.14
         assert def_frame.initial_frequency == 3.14
 
     def test_hardware_object(self, def_frame: DefFrame, hardware_object: Optional[str]):
-        assert def_frame.hardware_object == hardware_object
+        assert (
+            def_frame.hardware_object == hardware_object is None
+            if not hardware_object
+            else def_frame["HARDWARE-OBJECT"]
+        )
         def_frame.hardware_object = "bfg"
         assert def_frame.hardware_object == "bfg"
 
-    def test_hardware_object(self, def_frame: DefFrame, hardware_object: Optional[str]):
-        assert def_frame.hardware_object == hardware_object
+    def test_hardware_object_json(self, def_frame: DefFrame, hardware_object: Optional[str]):
+        assert (
+            def_frame.hardware_object == hardware_object is None
+            if not hardware_object
+            else def_frame["HARDWARE-OBJECT"]
+        )
         def_frame.hardware_object = '{"string": "str", "int": 1, "float": 3.14}'
         assert def_frame.hardware_object == '{"string": "str", "int": 1, "float": 3.14}'
 
     def test_sample_rate(self, def_frame: DefFrame, sample_rate: Optional[float]):
-        assert def_frame.sample_rate == sample_rate
+        assert def_frame.sample_rate == sample_rate is None if not sample_rate else def_frame["SAMPLE-RATE"]
         def_frame.sample_rate = 96.0
         assert def_frame.sample_rate == 96.0
 
     def test_center_frequency(self, def_frame: DefFrame, center_frequency: Optional[float]):
-        assert def_frame.center_frequency == center_frequency
+        assert (
+            def_frame.center_frequency == center_frequency is None
+            if not center_frequency
+            else def_frame.center_frequency
+        )
         def_frame.center_frequency = 432.0
         assert def_frame.center_frequency == 432.0