maintain positional arg order for imaging plane

Author: stephprince

src/pynwb/ophys.py

@@ -69,10 +69,10 @@ class ImagingPlane(NWBContainer):
             {'name': 'optical_channel', 'type': (list, OpticalChannel),  # required
              'doc': 'One of possibly many groups storing channel-specific data.'},
             {'name': 'description', 'type': str, 'doc': 'Description of this ImagingPlane.', 'default': None},
-            {'name': 'device', 'type': Device, 'doc': 'the device that was used to record'},  # required
-            {'name': 'excitation_lambda', 'type': float, 'doc': 'Excitation wavelength in nm.'},  # required
-            {'name': 'indicator', 'type': str, 'doc': 'Calcium indicator'},  # required
-            {'name': 'location', 'type': str, 'doc': 'Location of image plane.'},  # required
+            {'name': 'device', 'type': Device, 'doc': 'the device that was used to record', 'default': None},  # required
+            {'name': 'excitation_lambda', 'type': float, 'doc': 'Excitation wavelength in nm.',  'default': None},  # required
+            {'name': 'indicator', 'type': str, 'doc': 'Calcium indicator',  'default': None},  # required
+            {'name': 'location', 'type': str, 'doc': 'Location of image plane.', 'default': None},  # required
             {'name': 'imaging_rate', 'type': float,
              'doc': 'Rate images are acquired, in Hz. If the corresponding TimeSeries is present, the rate should be '
                     'stored there instead.', 'default': None},
@@ -128,6 +128,21 @@ def __init__(self, **kwargs):
 
         if not isinstance(args_to_set['optical_channel'], list):
             args_to_set['optical_channel'] = [args_to_set['optical_channel']]
+
+        # Note: device, excitation_lambda, indicator, and location are required arguments.
+        # Description was made to be optional in PyNWB 3.1.0, however to avoid breaking API changes, 
+        # the order of the arguments needs to be maintained even though the optional arguments came before the required ones.
+        # So in docval these required arguments are displayed as optional when really they are required. 
+        # This section can be removed when positional arguments are no longer allowed.
+        if args_to_set['device'] is None:
+            raise ValueError("The 'device' argument is required for ImagingPlane.")
+        if args_to_set['excitation_lambda'] is None:
+            raise ValueError("The 'excitation_lambda' argument is required for ImagingPlane.")
+        if args_to_set['indicator'] is None:
+            raise ValueError("The 'indicator' argument is required for ImagingPlane.")
+        if args_to_set['location'] is None:
+            raise ValueError("The 'location' argument is required for ImagingPlane.")
+
         if args_to_set['manifold'] is not None:
             error_msg = "The 'manifold' argument is deprecated in favor of 'origin_coords' and 'grid_spacing'."
             self._error_on_new_pass_on_construct(error_msg=error_msg)

tests/unit/test_ophys.py

@@ -197,25 +197,67 @@ def test_unit_deprecated(self):
         obj = ImagingPlane.__new__(ImagingPlane, in_construct_mode=True)
         obj.__init__(**kwargs)
 
-    def test_init_pos_args(self):
-        """Check creation of ImagingPlane with only required dependencies and positional kwargs.
+    def test_init_description_optional(self):
+        """Check creation of ImagingPlane with only required dependencies.
 
         This is to check how creation of an ImagingPlane changes when the "description" argument moves from a
         required arg in the middle of the required args section to an optional arg, in alignment with the schema.
         """
         oc, device = self.set_up_dependencies()
 
+        # description is now optional
         ip = ImagingPlane(
-            'test_imaging_plane',
-            oc,
-            device,
-            600.,
-            'indicator',
-            'location',
-        )
+                name='test_imaging_plane',
+                optical_channel=oc,
+                device=device,
+                excitation_lambda=600.,
+                indicator='indicator',
+                location='location',
+            )
         self.assertIsNone(ip.description)
-        self.assertIs(ip.device, device)
 
+        # description is still required to be provided when using positional arguments
+        with warnings.catch_warnings(record=True) as w:  # catch positional argument deprecation warning
+            with self.assertRaises(TypeError):
+                ImagingPlane(
+                    'test_imaging_plane',
+                    oc,
+                    device,
+                    600.,
+                    'indicator',
+                    'location',
+                )
+    
+    def test_init_missing_required_args(self):
+        """Check that ImagingPlane raises an error if required args are missing."""
+        oc, device = self.set_up_dependencies()
+
+        with self.assertRaisesWith(ValueError, "The 'device' argument is required for ImagingPlane."):
+            ImagingPlane(
+                name='test_imaging_plane',
+                optical_channel=oc,
+            )
+        with self.assertRaisesWith(ValueError, "The 'excitation_lambda' argument is required for ImagingPlane."):
+            ImagingPlane(
+                name='test_imaging_plane',
+                optical_channel=oc,
+                device=device,
+            )
+        with self.assertRaisesWith(ValueError, "The 'indicator' argument is required for ImagingPlane."):
+            ImagingPlane(
+                name='test_imaging_plane',
+                optical_channel=oc,
+                device=device,
+                excitation_lambda=600.,
+            )
+        with self.assertRaisesWith(ValueError, "The 'location' argument is required for ImagingPlane."):
+            ImagingPlane(
+                name='test_imaging_plane',
+                optical_channel=oc,
+                device=device,
+                excitation_lambda=600.,
+                indicator='indicator',
+            )
 
 class OnePhotonSeriesConstructor(TestCase):