Improve support for reading older files, format code

CHANGELOG.md

@@ -1,19 +1,25 @@
 # Changelog for ndx-pose
 
-## ndx-pose 0.2.0 (Upcoming)
-
-### Breaking changes
-- Removed the `nodes` and `edges` fields from `PoseEstimation` neurodata type. To specify these,
-  create a `Skeleton` object with those values, create a `Skeletons` object and pass the `Skeleton`
-  object to that, and add the `Skeletons` object to your "behavior" processing module. @rly (#7, #24)
+## ndx-pose 0.2.0 (July 19, 2024)
 
 ### Major changes
 - Added support for storing training data in the new `PoseTraining` neurodata type and other new types.
   @roomrys, @CBroz1, @rly, @talmo, @eberrigan (#7, #21, #24)
+- Deprecated the `nodes` and `edges` constructor arguments and fields from the `PoseEstimation` neurodata type.
+  Instead:
+  - Create a `Skeleton` object with those nodes and edges.
+  - Create a `Skeletons` container object and pass the `Skeleton` object to that.
+  - Add the `Skeletons` object to your "behavior" processing module (at the same level as the `PoseEstimation` object).
+  - Pass the `Skeleton` object to the `PoseEstimation` constructor.
+  Data from ndx-pose < 0.2.0 can still be read; a temporary `Skeleton` object with name "subject" will be created
+  and the `nodes` and `edges` fields will be populated with the data from the `PoseEstimation` object. @rly
+  (#7, #24, #33)
+- Added ability to link a `PoseEstimation` object to one or more camera devices. If the number of original videos,
+  labeled videos, or dimensions does not equal the number of camera devices when creating the object not from a file,
+  a `DeprecationWarning` will be raised. @rly (#33)
 
 ### Minor changes
 - Made `PoseEstimation.confidence` optional. @h-mayorquin (#11)
-- Added ability to link a `PoseEstimation` object to one or more camera devices.
 
 ## ndx-pose 0.1.1 (January 26, 2022)
 

pyproject.toml

@@ -98,6 +98,7 @@ exclude_lines = [
 [tool.black]
 line-length = 120
 preview = true
+enable-unstable-feature = ["string_processing"]
 exclude = ".git|.mypy_cache|.tox|.venv|venv|.ipynb_checkpoints|_build/|dist/|__pypackages__|.ipynb|docs/"
 
 [tool.ruff]

src/pynwb/ndx_pose/io/pose.py

@@ -23,3 +23,43 @@ def __init__(self, spec):
         super().__init__(spec)
         source_software_spec = self.spec.get_dataset("source_software")
         self.map_spec("source_software_version", source_software_spec.get_attribute("version"))
+
+    @NWBContainerMapper.constructor_arg("nodes")
+    def nodes(self, builder, manager):
+        """Set the constructor arg for 'nodes' to the value of the GroupBuilder dataset "nodes".
+
+        Used when constructing a PoseEstimation container from a written file.
+
+        ndx-pose 0.2.0 introduced a new attribute 'skeleton' to the PoseEstimation container. This Skeleton
+        container has two datasets, 'nodes' and 'edges', which were previously stored directly in the
+        PoseEstimation container. When data written with ndx-pose versions < 0.2.0 are read, the 'nodes' and
+        'edges' arguments in the PoseEstimation constructor are set to the values of the "nodes" and "edges"
+        DatasetBuilders read from the file. When data written with ndx-pose versions >= 0.2.0 are read,
+        'nodes' and 'edges' are set to None in the PoseEstimation constructor.
+        """
+        nodes_builder = builder.datasets.get("nodes")
+        if nodes_builder:
+            nodes = nodes_builder.data
+        else:
+            nodes = None
+        return nodes
+
+    @NWBContainerMapper.constructor_arg("edges")
+    def edges(self, builder, manager):
+        """Set the constructor arg for 'edges' to the value of the GroupBuilder dataset "edges".
+
+        Used when constructing a PoseEstimation container from a written file.
+
+        ndx-pose 0.2.0 introduced a new attribute 'skeleton' to the PoseEstimation container. This Skeleton
+        container has two datasets, 'nodes' and 'edges', which were previously stored directly in the
+        PoseEstimation container. When data written with ndx-pose versions < 0.2.0 are read, the 'nodes' and
+        'edges' arguments in the PoseEstimation constructor are set to the values of the "nodes" and "edges"
+        DatasetBuilders read from the file. When data written with ndx-pose versions >= 0.2.0 are read,
+        'nodes' and 'edges' are set to None in the PoseEstimation constructor.
+        """
+        edges_builder = builder.datasets.get("edges")
+        if edges_builder:
+            edges = edges_builder.data
+        else:
+            edges = None
+        return edges

src/pynwb/ndx_pose/pose.py

@@ -1,5 +1,4 @@
 import warnings
-
 from hdmf.utils import docval, popargs, get_docval, AllowPositional
 from pynwb import register_class, TimeSeries, get_class
 from pynwb.behavior import SpatialSeries
@@ -31,13 +30,13 @@ class PoseEstimationSeries(SpatialSeries):
         {
             "name": "name",
             "type": str,
-            "doc": ("Name of this PoseEstimationSeries, usually the name of a body part."),
+            "doc": "Name of this PoseEstimationSeries, usually the name of a body part.",
         },
         {
             "name": "data",
             "type": ("array_data", "data", TimeSeries),
             "shape": ((None, 2), (None, 3)),
-            "doc": ("Estimated position (x, y) or (x, y, z)."),
+            "doc": "Estimated position (x, y) or (x, y, z).",
         },
         {
             "name": "reference_frame",
@@ -48,7 +47,7 @@ class PoseEstimationSeries(SpatialSeries):
             "name": "confidence",
             "type": ("array_data", "data"),
             "shape": (None,),
-            "doc": ("Confidence or likelihood of the estimated positions, scaled to be between 0 and 1."),
+            "doc": "Confidence or likelihood of the estimated positions, scaled to be between 0 and 1.",
             "default": None,
         },
         {
@@ -64,9 +63,7 @@ class PoseEstimationSeries(SpatialSeries):
         {
             "name": "confidence_definition",
             "type": str,
-            "doc": (
-                "Description of how the confidence was computed, e.g., " "'Softmax output of the deep neural network'."
-            ),
+            "doc": "Description of how the confidence was computed, e.g., 'Softmax output of the deep neural network'.",
             "default": None,
         },
         *get_docval(
@@ -93,7 +90,7 @@ def __init__(self, **kwargs):
 
 
 @register_class("PoseEstimation", "ndx-pose")
-# NOTE NWB MultiContainerInterface extends NWBDataInterface and HDMF MultiContainerInterface
+# NOTE: NWB MultiContainerInterface extends NWBDataInterface and HDMF MultiContainerInterface
 class PoseEstimation(MultiContainerInterface):
     """Estimated position data for multiple body parts, computed from the same video with the same tool/algorithm.
     The timestamps of each child PoseEstimationSeries type should be the same.
@@ -122,83 +119,88 @@ class PoseEstimation(MultiContainerInterface):
         "source_software_version",
         "nodes",
         "edges",
-        "skeleton",
+        "skeleton",  # <-- this is a link to a Skeleton object
     )
 
     # custom mapper in ndx_pose.io.pose maps:
     # 'source_software' dataset -> 'version' attribute to 'source_software_version' field
     # if not for the custom mapper and custom validation, this class could be auto-generated from the spec
 
-    @docval(  # all fields optional
+    @docval(  # all fields are optional
         {
             "name": "pose_estimation_series",
             "type": ("array_data", "data"),
-            "doc": ("Estimated position data for each body part."),
+            "doc": "Estimated position data for each body part.",
             "default": None,
         },
         {
             "name": "name",
             "type": str,
-            "doc": ("Description of the pose estimation procedure and output."),
+            "doc": "Description of the pose estimation procedure and output.",
             "default": "PoseEstimation",
         },
         {
             "name": "description",
             "type": str,
-            "doc": ("Description of the pose estimation procedure and output."),
+            "doc": "Description of the pose estimation procedure and output.",
             "default": None,
         },
         {
             "name": "original_videos",
             "type": ("array_data", "data"),
             "shape": (None,),
-            "doc": (
-                "Paths to the original video files. The number of files should equal the number of camera devices."
-            ),
+            "doc": "Paths to the original video files. The number of files should equal the number of camera devices.",
             "default": None,
         },
         {
             "name": "labeled_videos",
             "type": ("array_data", "data"),
             "shape": (None,),
-            "doc": ("Paths to the labeled video files. The number of files should equal the number of camera devices."),
+            "doc": "Paths to the labeled video files. The number of files should equal the number of camera devices.",
             "default": None,
         },
         {
             "name": "dimensions",
             "type": ("array_data", "data"),
             "shape": ((None, 2)),
-            "doc": ("Dimensions of each labeled video file."),
+            "doc": (
+                "Dimensions of each labeled video file. The number of dimension pairs should equal the number of "
+                "camera devices."
+            ),
             "default": None,
         },
         {
             "name": "devices",
             "type": ("array_data", "data"),
-            "doc": ("Cameras used to record the videos."),
+            "doc": "Cameras used to record the videos.",
             "default": None,
         },
         {
             "name": "scorer",
             "type": str,
-            "doc": ("Name of the scorer / algorithm used."),
+            "doc": "Name of the scorer / algorithm used.",
             "default": None,
         },
         {
             "name": "source_software",
             "type": str,
-            "doc": ("Name of the software tool used. Specifying the version attribute is strongly encouraged."),
+            "doc": "Name of the software tool used. Specifying the version attribute is strongly encouraged.",
             "default": None,
         },
         {
             "name": "source_software_version",
             "type": str,
-            "doc": ("Version string of the software tool used."),
+            "doc": "Version string of the software tool used.",
             "default": None,
         },
         {
             "name": "skeleton",
             "type": Skeleton,
-            "doc": ("Layout of body part locations and connections."),
+            "doc": (
+                "Layout of body part locations and connections. The Skeleton object should be placed in a "
+                "Skeletons object which resides in the NWBFile at the same level as the PoseEstimation object. "
+                "The Skeleton object should be linked here."
+            ),
             "default": None,
         },
         {
@@ -227,13 +229,19 @@ def __init__(self, **kwargs):
         nodes, edges, skeleton = popargs("nodes", "edges", "skeleton", kwargs)
         if nodes is not None or edges is not None:
             if skeleton is not None:
-                raise ValueError("Cannot specify both 'nodes' and 'edges' and 'skeleton'.")
+                raise ValueError("Cannot specify 'skeleton' with 'nodes' or 'edges'.")
+            # TODO: this Skeleton is normally a link to a Skeleton elsewhere in the file (e.g., in a Skeletons object)
+            # Here, the Skeleton is constructed from the nodes and edges and exists only in this PoseEstimation object
+            # and not as a child; this can have unintended consequences if the file is rewritten with the latest
+            # schema. This is a limitation of the current implementation, and will be addressed in a future release.
             skeleton = Skeleton(name="subject", nodes=nodes, edges=edges)
-            warnings.warn(
-                "The 'nodes' and 'edges' arguments are deprecated. Please use the 'skeleton' argument instead.",
-                DeprecationWarning,
-                stacklevel=2,
-            )
+            # warn on new, no warning on construction from existing file
+            if not self._in_construct_mode:
+                msg = (
+                    "The 'nodes' and 'edges' constructor arguments are deprecated. Please use the 'skeleton' "
+                    "argument instead. These will be removed in a future release."
+                )
+                warnings.warn(msg, DeprecationWarning)
 
         # devices must be added to the NWBFile before being linked to from a PoseEstimation object.
         # otherwise, they will be added as children of the PoseEstimation object.
@@ -245,11 +253,39 @@ def __init__(self, **kwargs):
                         "All devices linked to from a PoseEstimation object must be added to the NWBFile first."
                     )
 
+        # validate that if original videos, labeled videos, or dimensions are provided, then an equal number of
+        # camera devices must be provided.
+        # specification of cameras was not allowed in ndx-pose 0.1.*.
+        # warn on new, no warning on construction from existing file
+        # TODO in a future release, change DeprecationWarning to a ValueError
+        original_videos, labeled_videos, dimensions = popargs("original_videos", "labeled_videos", "dimensions", kwargs)
+        if original_videos is not None and (devices is None or len(original_videos) != len(devices)):
+            if not self._in_construct_mode:
+                msg = (
+                    "The number of original videos must equal the number of camera devices. This will become an error "
+                    "in a future release."
+                )
+                raise DeprecationWarning(msg)
+        if labeled_videos is not None and (devices is None or len(labeled_videos) != len(devices)):
+            if not self._in_construct_mode:
+                msg = (
+                    "The number of labeled videos must equal the number of camera devices. This will become an error "
+                    "in a future release."
+                )
+                raise DeprecationWarning(msg)
+        if dimensions is not None and (devices is None or len(dimensions) != len(devices)):
+            if not self._in_construct_mode:
+                msg = (
+                    "The number of dimensions must equal the number of camera devices. This will become an error in a "
+                    "future release."
+                )
+                raise DeprecationWarning(msg)
+
         pose_estimation_series, description = popargs("pose_estimation_series", "description", kwargs)
-        original_videos, labeled_videos = popargs("original_videos", "labeled_videos", kwargs)
-        dimensions, scorer = popargs("dimensions", "scorer", kwargs)
+        scorer = popargs("scorer", kwargs)
         source_software, source_software_version = popargs("source_software", "source_software_version", kwargs)
         super().__init__(**kwargs)
+
         self.pose_estimation_series = pose_estimation_series
         self.description = description
         self.original_videos = original_videos
@@ -260,33 +296,26 @@ def __init__(self, **kwargs):
         self.source_software = source_software
         self.source_software_version = source_software_version
         self.skeleton = skeleton
-        self.fields["nodes"] = nodes
-        self.fields["edges"] = edges
 
         # TODO include calibration images for 3D estimates?
-
         # TODO validate that the nodes correspond to the names of the pose estimation series objects
 
-        # validate that len(original_videos) == len(labeled_videos) == len(dimensions) == len(cameras)
-        if original_videos is not None and (devices is None or len(original_videos) != len(devices)):
-            raise ValueError("The number of original videos should equal the number of camera devices.")
-        if labeled_videos is not None and (devices is None or len(labeled_videos) != len(devices)):
-            raise ValueError("The number of labeled videos should equal the number of camera devices.")
-        if dimensions is not None and (devices is None or len(dimensions) != len(devices)):
-            raise ValueError("The number of dimensions should equal the number of camera devices.")
-
     @property
     def nodes(self):
         return self.skeleton.nodes
 
     @nodes.setter
     def nodes(self, value):
-        raise ValueError("'nodes' is deprecated. Please use the 'skeleton' field instead.")
+        raise ValueError(
+            "Setting PoseEstimation.nodes is deprecated. Please use PoseEstimation.skeleton.nodes instead."
+        )
 
     @property
     def edges(self):
         return self.skeleton.edges
 
     @edges.setter
     def edges(self, value):
-        raise ValueError("'edges' is deprecated. Please use the 'skeleton' field instead.")
+        raise ValueError(
+            "Setting PoseEstimation.edges is deprecated. Please use PoseEstimation.skeleton.edges instead."
+        )