Three more annotations that described the wrong thing, one of them a live bug

musicalgestures/_flow.py

@@ -30,6 +30,26 @@ def __init__(self, parent: "musicalgestures.MgVideo", filename, color, has_audio
         self.filename = filename
         self.color = color
         self.has_audio = has_audio
+
+    def _parent(self) -> "musicalgestures.MgVideo":
+        """The MgVideo this Flow belongs to.
+
+        `self.parent` is a weak reference, so it resolves to None once the video
+        is garbage collected --- which happens with
+        `flow = mg.MgVideo(...).flow`, where nothing keeps the video alive past
+        the expression. Dereferencing it then produced an AttributeError naming
+        whichever attribute was reached for, which says nothing about the cause.
+
+        Raises:
+            RuntimeError: If the parent video no longer exists.
+        """
+        parent = self.parent()
+        if parent is None:
+            raise RuntimeError(
+                "the MgVideo this Flow belongs to no longer exists. Keep a reference to the "
+                "video rather than to its flow: `mv = mg.MgVideo(...); mv.flow.dense()`, not "
+                "`flow = mg.MgVideo(...).flow`.")
+        return parent
 
     def dense(
             self,
@@ -88,13 +108,13 @@ def dense(
         # Convert to avi if the input is not avi - necesarry for cv2 compatibility on all platforms
         if convert and fex != '.avi':
             # first check if there already is a converted version, if not create one and register it to the parent self
-            if "as_avi" not in self.parent().__dict__.keys():
+            if "as_avi" not in self._parent().__dict__.keys():
                 file_as_avi = convert_to_avi(of + fex, overwrite=overwrite)
                 # register it as the avi version for the file
-                self.parent().as_avi = musicalgestures.MgVideo(file_as_avi)
+                self._parent().as_avi = musicalgestures.MgVideo(file_as_avi)
             # point of and fex to the avi version
-            of, fex = self.parent().as_avi.of, self.parent().as_avi.fex
-            filename = self.parent().as_avi.filename
+            of, fex = self._parent().as_avi.of, self._parent().as_avi.fex
+            filename = self._parent().as_avi.filename
 
         vidcap = cv2.VideoCapture(filename)
 
@@ -274,10 +294,10 @@ def dense(
                 os.remove(source_audio)
 
             # save result at flow_dense_video at parent MgVideo
-            self.parent().flow_dense_video = musicalgestures.MgVideo(
+            self._parent().flow_dense_video = musicalgestures.MgVideo(
                 destination_video, color=self.color, returned_by_process=True)
 
-            return self.parent().flow_dense_video
+            return self._parent().flow_dense_video
 
     def get_acceleration(self, velocity: list, fps: int):
 
@@ -347,13 +367,13 @@ def sparse(
         # Convert to avi if the input is not avi - necesarry for cv2 compatibility on all platforms
         if convert and fex != '.avi':
             # first check if there already is a converted version, if not create one and register it to the parent self
-            if "as_avi" not in self.parent().__dict__.keys():
+            if "as_avi" not in self._parent().__dict__.keys():
                 file_as_avi = convert_to_avi(of + fex, overwrite=overwrite)
                 # register it as the avi version for the file
-                self.parent().as_avi = musicalgestures.MgVideo(file_as_avi)
+                self._parent().as_avi = musicalgestures.MgVideo(file_as_avi)
             # point of and fex to the avi version
-            of, fex = self.parent().as_avi.of, self.parent().as_avi.fex
-            filename = self.parent().as_avi.filename
+            of, fex = self._parent().as_avi.of, self._parent().as_avi.fex
+            filename = self._parent().as_avi.filename
 
         vidcap = cv2.VideoCapture(filename)
         ret, frame = vidcap.read()
@@ -487,7 +507,7 @@ def sparse(
             os.remove(source_audio)
 
         # save result at flow_sparse_video at parent MgVideo
-        self.parent().flow_sparse_video = musicalgestures.MgVideo(
+        self._parent().flow_sparse_video = musicalgestures.MgVideo(
             destination_video, color=self.color, returned_by_process=True)
 
-        return self.parent().flow_sparse_video
+        return self._parent().flow_sparse_video

musicalgestures/_motionvideo.py

@@ -474,7 +474,7 @@ def mg_motiondata(
         motion_analysis: str = 'all',
         data_format: str | list = "csv",
         target_name: str | None = None,
-        overwrite: bool = True) -> "list":
+        overwrite: bool = True) -> "str | list":
     """
     Shortcut for `mg_motion` to only render motion data.
 
@@ -494,7 +494,7 @@ def mg_motiondata(
         str/list: The path(s) to the rendered data file(s).
     """
 
-    out = None
+    out: "str | list | None" = None
 
     if type(data_format) == str:
         if target_name is None:
@@ -505,6 +505,7 @@ def mg_motiondata(
 
     if type(data_format) == list:
         out = []
+        paths: list = out
         if target_name is None:
             # this csv is just a temporary placeholder, the correct extension is always enforced based on the data_format(s)
             target_name = self.of + '_motion.csv'
@@ -514,7 +515,7 @@ def mg_motiondata(
                 tmp_name = generate_outfilename(target_name_of + '.' + item)
             else:
                 tmp_name = target_name_of + '.' + item
-            out.append(tmp_name)
+            paths.append(tmp_name)
 
     mg_motion(
         self,
@@ -538,6 +539,7 @@ def mg_motiondata(
     #     return outlist
     # else:
     #     return self.of + '_motion.' + data_format
+    assert out is not None, f"unsupported data_format: {data_format!r}"
     return out
 
 

musicalgestures/_utils.py

@@ -578,13 +578,13 @@ def get_frame_planecount(frame: np.ndarray) -> int:
     return 3 if len(np.array(frame).shape) == 3 else 1
 
 
-def frame2ms(frame: int, fps: int) -> int:
+def frame2ms(frame: int, fps: float) -> int:
     """
     Converts frames to milliseconds.
 
     Args:
         frame (int): The index of the frame to be converted to milliseconds.
-        fps (int): Frames per second.
+        fps (float): Frames per second. Not an integer: 29.97 and 23.976 are ordinary.
 
     Returns:
         int: The rounded millisecond value of the input frame index.

musicalgestures/_video.py

@@ -97,7 +97,13 @@ def __init__(
             self.filename = filename
 
         self.array = array
-        self.fps = fps
+        # The ARGUMENT is optional: it is the rate an array is encoded at, and a file
+        # carries its own. The ATTRIBUTE is not, because `get_video()` below reads the
+        # true rate from the file and overwrites this, so by the end of `__init__` it is
+        # always a number. 0.0 stands for "not given yet" over the few lines before that
+        # happens, and the from-array branch below tests it for truth rather than for
+        # None, which rejects an explicit `fps=0` --- not a frame rate anything can use.
+        self.fps: float = fps if fps is not None else 0.0
         self.path = path
         # Name of file without extension (only-filename)
         self.of = os.path.splitext(self.filename)[0]
@@ -126,7 +132,16 @@ def __init__(
         # Check input and if FFmpeg is properly installed
         self.test_input()
 
-        if self.array is not None and self.fps is not None:
+        if self.array is not None and not self.fps:
+            # Without a rate there is nothing to encode the array at, so the write below
+            # never happens and the read after it fails with "no such file" --- blaming
+            # the output for a missing argument. Both before and after this attribute
+            # stopped being Optional, so say what is actually wrong.
+            raise ValueError(
+                "fps= is required when MgVideo is given an array: an array carries no frame "
+                "rate of its own, so there is nothing to encode it at.")
+
+        if self.array is not None and self.fps:
             self.from_numpy(self.array, self.fps)
         elif fps is not None:
             # `fps` is the rate an ARRAY is encoded at. For a file input `get_video()` below reads

tests/test_flow_parent.py

@@ -0,0 +1,89 @@
+"""A Flow holds its video by weak reference, so it can outlive it.
+
+`MgVideo` keeps a strong reference to its `Flow` and the `Flow` keeps a weak one
+back, which avoids a cycle. It also means `flow = mg.MgVideo(path).flow` leaves
+nothing holding the video: it is collected at the end of the expression and the
+flow's `self.parent()` resolves to None from then on. Dereferencing that raised
+an AttributeError naming whichever attribute was reached for, which said nothing
+about the cause.
+"""
+import gc
+
+import pytest
+
+import musicalgestures
+from musicalgestures._flow import Flow
+
+
+class _Detached:
+    """A Flow whose parent is already gone, without needing a real video."""
+
+    def __init__(self):
+        import weakref
+
+        class _Doomed:
+            pass
+
+        doomed = _Doomed()
+        self.flow = Flow.__new__(Flow)
+        self.flow.parent = weakref.ref(doomed)
+        del doomed
+        gc.collect()
+
+
+class TestParentResolution:
+    def test_a_live_parent_is_returned(self):
+        import weakref
+
+        class _Alive:
+            pass
+
+        alive = _Alive()
+        flow = Flow.__new__(Flow)
+        flow.parent = weakref.ref(alive)
+        assert flow._parent() is alive
+
+    def test_a_collected_parent_raises_something_explaining_itself(self):
+        flow = _Detached().flow
+        with pytest.raises(RuntimeError) as excinfo:
+            flow._parent()
+        message = str(excinfo.value)
+        assert "no longer exists" in message
+        assert "mv.flow.dense()" in message, "the message must show the working pattern"
+
+    def test_the_failure_is_not_an_attributeerror(self):
+        """An AttributeError here names an attribute, not the reason."""
+        flow = _Detached().flow
+        with pytest.raises(RuntimeError):
+            flow._parent()
+
+
+class TestFlowIsStillWeak:
+    """The weak reference is deliberate. A well-meaning change to a strong one
+    would make every MgVideo that touched .flow immortal, since the video holds
+    the flow and the flow would hold the video."""
+
+    def test_the_stored_parent_is_a_weak_reference_not_the_video(self):
+        import weakref
+
+        class _Video:
+            pass
+
+        video = _Video()
+        flow = Flow.__new__(Flow)
+        Flow.__init__(flow, video, filename="f.avi", color=True, has_audio=False)
+        assert isinstance(flow.parent, weakref.ref)
+        assert flow.parent() is video
+
+    def test_the_flow_does_not_keep_the_video_alive(self):
+        import weakref
+
+        class _Video:
+            pass
+
+        video = _Video()
+        flow = Flow.__new__(Flow)
+        Flow.__init__(flow, video, filename="f.avi", color=True, has_audio=False)
+        del video
+        gc.collect()
+        assert flow.parent() is None, "the flow kept its video alive"

tests/test_result_attributes.py

@@ -110,3 +110,32 @@ def test_x_maps_to_vertical_and_y_to_horizontal(self, kind):
             setattr(v, f"{kind}_y", "from-y")
         assert getattr(v, f"{kind}_vertical_image") == "from-x"
         assert getattr(v, f"{kind}_horizontal_image") == "from-y"
+
+
+class TestFpsIsAlwaysANumber:
+    """`self.fps` is a float, not `float | None`.
+
+    The ARGUMENT is optional, because a file carries its own rate and only an
+    array needs one supplied. The ATTRIBUTE is not: `get_video()` reads the true
+    rate from the file and overwrites whatever the constructor stored, so by the
+    end of `__init__` it is always a number. Saying so closed twelve mypy errors
+    across the modules that divide by it.
+    """
+
+    def test_a_constructed_video_has_a_real_rate(self, tmp_path):
+        import numpy as np
+
+        arr = np.zeros((6, 16, 16, 3), dtype=np.uint8)
+        v = mg.MgVideo(filename=str(tmp_path / "rate.avi"), array=arr, fps=24)
+        assert isinstance(v.fps, float)
+        assert v.fps > 0
+
+    def test_an_array_without_a_rate_says_so(self):
+        """It used to fail with "no such file", blaming the output for a
+        missing argument, because the encode never ran."""
+        import numpy as np
+
+        arr = np.zeros((4, 8, 8, 3), dtype=np.uint8)
+        for kwargs in ({}, {"fps": 0}):
+            with pytest.raises(ValueError, match="fps= is required"):
+                mg.MgVideo(filename="unused.avi", array=arr, **kwargs)