Merge branch 'dev' into better_fix_curl

Author: CodyCBakerPhD

nwbinspector/internal_configs/dandi.inspector_config.yaml

@@ -2,7 +2,7 @@ CRITICAL:  # All the fields under CRITICAL will be required for dandi validate t
   - check_subject_exists
   - check_subject_id_exists
 BEST_PRACTICE_VIOLATION:
-  - check_subject_sex # these are planned to be elevated to CRITICAL when requried for DANDI validate
-  - check_subject_species # these are planned to be elevated to CRITICAL when requried for DANDI validate
-  - check_subject_age # these are planned to be elevated to CRITICAL when requried for DANDI validate
+  - check_subject_sex # these are planned to be elevated to CRITICAL when required for DANDI validate
+  - check_subject_species # these are planned to be elevated to CRITICAL when required for DANDI validate
+  - check_subject_age # these are planned to be elevated to CRITICAL when required for DANDI validate
   - check_data_orientation  # not 100% accurate, so need to deelevate from CRITICAL to skip it in dandi validate

nwbinspector/nwbinspector.py

@@ -13,12 +13,13 @@
 from types import FunctionType
 from warnings import filterwarnings, warn
 from distutils.util import strtobool
-from time import sleep
+from collections import defaultdict
 
 import click
 import pynwb
 import yaml
 from tqdm import tqdm
+from natsort import natsorted
 
 from . import available_checks
 from .inspector_tools import (
@@ -37,7 +38,7 @@
 class InspectorOutputJSONEncoder(json.JSONEncoder):
     """Custom JSONEncoder for the NWBInspector."""
 
-    def default(self, o):
+    def default(self, o):  # noqa D102
         if isinstance(o, InspectorMessage):
             return o.__dict__
         if isinstance(o, Enum):
@@ -53,25 +54,41 @@ def validate_config(config: dict):
     jsonschema.validate(instance=config, schema=schema)
 
 
-def copy_function(function):
+def _copy_function(function):
     """
-    Return a copy of a function so that internal attributes can be adjusted without changing the original function.
+    Copy the core parts of a given function, excluding wrappers, then return a new function.
 
-    Required to ensure our configuration of functions in the registry does not effect the registry itself.
-
-    Taken from
+    Based off of
     https://stackoverflow.com/questions/6527633/how-can-i-make-a-deepcopy-of-a-function-in-python/30714299#30714299
     """
-    if getattr(function, "__wrapped__", False):
-        function = function.__wrapped__
     copied_function = FunctionType(
         function.__code__, function.__globals__, function.__name__, function.__defaults__, function.__closure__
     )
-    # in case f was given attrs (note this dict is a shallow copy):
+
+    # in case f was given attrs (note this dict is a shallow copy)
     copied_function.__dict__.update(function.__dict__)
     return copied_function
 
 
+def copy_check(function):
+    """
+    Copy a check function so that internal attributes can be adjusted without changing the original function.
+
+    Required to ensure our configuration of functions in the registry does not effect the registry itself.
+
+    Also copies the wrapper for auto-parsing ressults,
+    see https://github.com/NeurodataWithoutBorders/nwbinspector/pull/218 for explanation.
+
+    Taken from
+    https://stackoverflow.com/questions/6527633/how-can-i-make-a-deepcopy-of-a-function-in-python/30714299#30714299
+    """
+    if getattr(function, "__wrapped__", False):
+        check_function = function.__wrapped__
+    copied_function = _copy_function(function)
+    copied_function.__wrapped__ = _copy_function(check_function)
+    return copied_function
+
+
 def load_config(filepath_or_keyword: PathType) -> dict:
     """
     Load a config dictionary either via keyword search of the internal configs, or an explicit filepath.
@@ -131,7 +148,7 @@ def configure_checks(
         checks_out = []
         ignore = ignore or []
         for check in checks:
-            mapped_check = copy_function(check)
+            mapped_check = copy_check(check)
             for importance_name, func_names in config.items():
                 if check.__name__ in func_names:
                     if importance_name == "SKIP":
@@ -370,6 +387,30 @@ def inspect_all(
     # Filtering of checks should apply after external modules are imported, in case those modules have their own checks
     checks = configure_checks(config=config, ignore=ignore, select=select, importance_threshold=importance_threshold)
 
+    # Manual identifier check over all files in the folder path
+    identifiers = defaultdict(list)
+    for nwbfile_path in nwbfiles:
+        with pynwb.NWBHDF5IO(path=nwbfile_path, mode="r", driver=driver) as io:
+            nwbfile = robust_s3_read(io.read)
+            identifiers[nwbfile.identifier].append(nwbfile_path)
+    if len(identifiers) != len(nwbfiles):
+        for identifier, nwbfiles_with_identifier in identifiers.items():
+            if len(nwbfiles_with_identifier) > 1:
+                yield InspectorMessage(
+                    message=(
+                        f"The identifier '{identifier}' is used across the .nwb files: "
+                        f"{natsorted([x.name for x in nwbfiles_with_identifier])}. "
+                        "The identifier of any NWBFile should be a completely unique value - "
+                        "we recommend using uuid4 to achieve this."
+                    ),
+                    importance=Importance.CRITICAL,
+                    check_function_name="check_unique_identifiers",
+                    object_type="NWBFile",
+                    object_name="root",
+                    location="/",
+                    file_path=str(path),
+                )
+
     nwbfiles_iterable = nwbfiles
     if progress_bar:
         nwbfiles_iterable = tqdm(nwbfiles_iterable, **progress_bar_options)

requirements.txt

@@ -2,3 +2,6 @@ pynwb
 PyYAML
 jsonschema
 packaging
+natsort
+click
+tqdm

setup.py

@@ -1,11 +1,14 @@
 from setuptools import setup, find_packages
+from pathlib import Path
 
-# Get the long description from the README file
-with open("README.md", "r") as f:
+root = Path(__file__).parent
+with open(root / "README.md", "r") as f:
     long_description = f.read()
+with open(root / "requirements.txt") as f:
+    install_requires = f.readlines()
 setup(
     name="nwbinspector",
-    version="0.4.9",
+    version="0.4.10",
     description="Tool to inspect NWB files for best practices compliance.",
     long_description=long_description,
     long_description_content_type="text/markdown",
@@ -14,7 +17,7 @@
     packages=find_packages(),
     include_package_data=True,
     url="https://github.com/NeurodataWithoutBorders/nwbinspector",
-    install_requires=["pynwb", "natsort", "click", "PyYAML", "jsonschema", "tqdm"],
+    install_requires=install_requires,
     extras_require=dict(dandi=["dandi>=0.39.2"]),
     entry_points={"console_scripts": ["nwbinspector=nwbinspector.nwbinspector:inspect_all_cli"]},
     license="BSD-3-Clause",

tests/test_check_configuration.py

@@ -9,7 +9,7 @@
     check_timestamps_match_first_dimension,
     available_checks,
 )
-from nwbinspector.nwbinspector import validate_config, configure_checks, copy_function, load_config
+from nwbinspector.nwbinspector import validate_config, configure_checks, _copy_function, load_config
 
 
 class TestCheckConfiguration(TestCase):
@@ -24,7 +24,7 @@ def setUpClass(cls):
 
     def test_safe_check_copy(self):
         initial_importance = available_checks[0].importance
-        changed_check = copy_function(function=available_checks[0])
+        changed_check = _copy_function(function=available_checks[0])
         if initial_importance is Importance.CRITICAL:
             changed_importance = Importance.BEST_PRACTICE_SUGGESTION
         else:

tests/test_inspector.py

@@ -4,21 +4,25 @@
 from tempfile import mkdtemp
 from pathlib import Path
 from unittest import TestCase
+from datetime import datetime
 
 import numpy as np
 from pynwb import NWBFile, NWBHDF5IO, TimeSeries
 from pynwb.file import TimeIntervals
 from pynwb.behavior import SpatialSeries, Position
 from hdmf.common import DynamicTable
+from natsort import natsorted
 
 from nwbinspector import (
     Importance,
     check_small_dataset_compression,
     check_regular_timestamps,
     check_data_orientation,
     check_timestamps_match_first_dimension,
+    check_subject_exists,
+    load_config,
 )
-from nwbinspector.nwbinspector import inspect_all, inspect_nwb
+from nwbinspector import inspect_all, inspect_nwb
 from nwbinspector.register_checks import Severity, InspectorMessage, register_check
 from nwbinspector.utils import FilePathType, is_module_installed
 from nwbinspector.tools import make_minimal_nwbfile
@@ -144,7 +148,7 @@ def assertLogFileContentsEqual(
                     if ".nwb" in test_line:
                         # Transform temporary testing path and formatted to hardcoded fake path
                         str_loc = test_line.find(".nwb")
-                        correction_str = test_line.replace(test_line[5 : str_loc - 8], "./")
+                        correction_str = test_line.replace(test_line[5 : str_loc - 8], "./")  # noqa: E203 (black)
                         test_file_lines[line_number] = correction_str
                 self.assertEqual(first=test_file_lines[skip_first_n_lines:-1], second=true_file_lines)
 
@@ -501,6 +505,71 @@ def test_inspect_nwb_manual_iteration_stop(self):
         with self.assertRaises(expected_exception=StopIteration):
             next(generator)
 
+    def test_inspect_nwb_dandi_config(self):
+        config_checks = [check_subject_exists] + self.checks
+        test_results = list(
+            inspect_nwb(
+                nwbfile_path=self.nwbfile_paths[0],
+                checks=config_checks,
+                config=load_config(filepath_or_keyword="dandi"),
+            )
+        )
+        true_results = [
+            InspectorMessage(
+                message="Subject is missing.",
+                importance=Importance.BEST_PRACTICE_SUGGESTION,
+                check_function_name="check_subject_exists",
+                object_type="NWBFile",
+                object_name="root",
+                location="/",
+                file_path=self.nwbfile_paths[0],
+            ),
+            InspectorMessage(
+                message="data is not compressed. Consider enabling compression when writing a dataset.",
+                importance=Importance.BEST_PRACTICE_SUGGESTION,
+                check_function_name="check_small_dataset_compression",
+                object_type="TimeSeries",
+                object_name="test_time_series_1",
+                location="/acquisition/test_time_series_1",
+                file_path=self.nwbfile_paths[0],
+            ),
+            InspectorMessage(
+                message=(
+                    "TimeSeries appears to have a constant sampling rate. "
+                    "Consider specifying starting_time=1.2 and rate=2.0 instead of timestamps."
+                ),
+                importance=Importance.BEST_PRACTICE_VIOLATION,
+                check_function_name="check_regular_timestamps",
+                object_type="TimeSeries",
+                object_name="test_time_series_2",
+                location="/acquisition/test_time_series_2",
+                file_path=self.nwbfile_paths[0],
+            ),
+            InspectorMessage(
+                message=(
+                    "Data may be in the wrong orientation. "
+                    "Time should be in the first dimension, and is usually the longest dimension. "
+                    "Here, another dimension is longer."
+                ),
+                importance=Importance.CRITICAL,
+                check_function_name="check_data_orientation",
+                object_type="SpatialSeries",
+                object_name="my_spatial_series",
+                location="/processing/behavior/Position/my_spatial_series",
+                file_path=self.nwbfile_paths[0],
+            ),
+            InspectorMessage(
+                message="The length of the first dimension of data does not match the length of timestamps.",
+                importance=Importance.CRITICAL,
+                check_function_name="check_timestamps_match_first_dimension",
+                object_type="TimeSeries",
+                object_name="test_time_series_3",
+                location="/acquisition/test_time_series_3",
+                file_path=self.nwbfile_paths[0],
+            ),
+        ]
+        self.assertCountEqual(first=test_results, second=true_results)
+
 
 @pytest.mark.skipif(not HAVE_ROS3 or not HAVE_DANDI, reason="Needs h5py setup with ROS3.")
 def test_dandiset_streaming():
@@ -561,3 +630,74 @@ def test_dandiset_streaming_cli_parallel(self):
             f"> {console_output_file}"
         )
         self.assertFileExists(path=self.tempdir / "test_nwbinspector_streaming_report_7.txt")
+
+
+class TestCheckUniqueIdentifiersPass(TestCase):
+    maxDiff = None
+
+    @classmethod
+    def setUpClass(cls):
+        cls.tempdir = Path(mkdtemp())
+        num_nwbfiles = 3
+        unique_id_nwbfiles = list()
+        for j in range(num_nwbfiles):
+            unique_id_nwbfiles.append(make_minimal_nwbfile())
+
+        cls.unique_id_nwbfile_paths = [str(cls.tempdir / f"unique_id_testing{j}.nwb") for j in range(num_nwbfiles)]
+        for nwbfile_path, nwbfile in zip(cls.unique_id_nwbfile_paths, unique_id_nwbfiles):
+            with NWBHDF5IO(path=nwbfile_path, mode="w") as io:
+                io.write(nwbfile)
+
+    @classmethod
+    def tearDownClass(cls):
+        rmtree(cls.tempdir)
+
+    def test_check_unique_identifiers_pass(self):
+        assert list(inspect_all(path=self.tempdir, select=["check_data_orientation"])) == []
+
+
+class TestCheckUniqueIdentifiersFail(TestCase):
+    maxDiff = None
+
+    @classmethod
+    def setUpClass(cls):
+        cls.tempdir = Path(mkdtemp())
+        num_nwbfiles = 3
+        non_unique_id_nwbfiles = list()
+        for j in range(num_nwbfiles):
+            non_unique_id_nwbfiles.append(
+                NWBFile(
+                    session_description="",
+                    identifier="not a unique identifier!",
+                    session_start_time=datetime.now().astimezone(),
+                )
+            )
+
+        cls.non_unique_id_nwbfile_paths = [
+            str(cls.tempdir / f"non_unique_id_testing{j}.nwb") for j in range(num_nwbfiles)
+        ]
+        for nwbfile_path, nwbfile in zip(cls.non_unique_id_nwbfile_paths, non_unique_id_nwbfiles):
+            with NWBHDF5IO(path=nwbfile_path, mode="w") as io:
+                io.write(nwbfile)
+
+    @classmethod
+    def tearDownClass(cls):
+        rmtree(cls.tempdir)
+
+    def test_check_unique_identifiers_fail(self):
+        assert list(inspect_all(path=self.tempdir, select=["check_data_orientation"])) == [
+            InspectorMessage(
+                message=(
+                    "The identifier 'not a unique identifier!' is used across the .nwb files: "
+                    f"{natsorted([Path(x).name for x in self.non_unique_id_nwbfile_paths])}. "
+                    "The identifier of any NWBFile should be a completely unique value - "
+                    "we recommend using uuid4 to achieve this."
+                ),
+                importance=Importance.CRITICAL,
+                check_function_name="check_unique_identifiers",
+                object_type="NWBFile",
+                object_name="root",
+                location="/",
+                file_path=str(self.tempdir),
+            )
+        ]

tests/unit_tests/test_tables.py

@@ -229,8 +229,8 @@ def test_check_single_row_pass():
 
 def test_check_single_row_ignore_units():
     table = Units(
-        name="Units",  # default name when building through nwbfile
-    )
+        name="Units",
+    )  # default name when building through nwbfile
     table.add_unit(spike_times=[1, 2, 3])
     assert check_single_row(table=table) is None