Merge branch 'dev' into missing_unit

Author: CodyCBakerPhD

docs/best_practices/best_practices_index.rst

@@ -20,11 +20,11 @@ Authors: Oliver Ruebel, Andrew Tritt, Ryan Ly, Cody Baker and Ben Dichter
 .. toctree::
    :maxdepth: 2
 
+   general
    nwbfile_metadata
    time_series
    tables
    ecephys
    ogen
-   naming
    simulated_data
    extensions

docs/best_practices/naming.rst renamed to docs/best_practices/general.rst

@@ -1,10 +1,15 @@
-Naming
-======
+General
+=======
 
 
-Neurodata Types
----------------
 
+Names
+-----
+
+
+
+Standard Names
+~~~~~~~~~~~~~~
 
 As a default, name class instances with the same name as the class.
 
@@ -22,32 +27,36 @@ Group. In this case the instances must have unique names. If they are both equal
 the class name (e.g. "ElectricalSeries_1" and "ElectricalSeries_2"). If one of the instances is an extra of less
 importance, name that one something different (e.g. "ElectricalSeries" and "ElectricalSeries_extra_electrode").
 
-Names are not for storing meta-data. If you need to place other data of the same neurodata_type, you will need to
-choose another name. Keep in mind that meta-data should not be stored solely in the name of objects. It is OK to name
-an object something like “ElectricalSeries_large_array” however the name alone is not sufficient documentation. In this
-case, the source of the signal will be clear from the device of the rows from the linked electrodes table region, and
-you should also include any important distinguishing information in the description field of the object. Make an effort
-to make meta-data as explicit as possible. Good names help users but do not help applications parse your file.
 
-’/’ is not allowed in names. When creating a custom name, using the forward slash (/) is not allowed, as this can
-confuse h5py and lead to the creation of an additional group. Instead of including a forward slash in the name, please
-use “Over” like in DfOverF.
 
+.. _best_practice_name_slashes:
+
+Do Not Use Slashes
+~~~~~~~~~~~~~~~~~~
+
+'/' is not allowed in Group or Dataset names, as this can confuse h5py and lead to the creation of an additional group.
+Instead of including a forward slash in the name, please use "Over" like in DfOverF.
 
-Processing Modules
-------------------
+Check function: :py:meth:`~nwbinspector.checks.general.check_name_slashes`
 
 
-Indicate Modality
-~~~~~~~~~~~~~~~~~
 
-Give preference to default processing module names. These names mirror the extension module names: “ecephys”,
-“icephys”, “behavior”, “ophys”, “misc”.
+Descriptions
+------------
 
-We encourage the use of these defaults, but there may be some cases when deviating from this pattern is appropriate.
 
-For instance, if there is a processing step that involves data from multiple modalities, or if the user wants to
-compare two processing pipelines for a single modality (e.g. different spike sorters), you should create
-Processing Modules with custom names.
 
-ProcessingModules are themselves neurodata_types, and the other rules for neurodata_types also apply here.
+.. _best_practice_description:
+
+Metadata and Descriptions
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Names are not for storing meta-data, so meta-data should not be stored solely in the name of objects. It is OK to name
+an object something like “ElectricalSeries_large_array” however the name alone is not sufficient documentation. In this
+case, the source of the signal will be clear from the device of the rows from the linked electrodes table region, and
+you should also include any important distinguishing information in the description field of the object. Make an effort
+to make meta-data as explicit as possible. Good names help users but do not help applications parse your file.
+
+As such, it is not recommended to use blank or default 'placeholder' descriptions.
+
+Check function: :py:meth:`~nwbinspector.checks.general.check_description`

nwbinspector/__init__.py

@@ -1,7 +1,8 @@
 from .register_checks import available_checks, Importance
 from .nwbinspector import inspect_nwb, inspect_all, load_config
+from .checks.general import *
+from .checks.nwbfile_metadata import *
 from .checks.nwb_containers import *
 from .checks.time_series import *
 from .checks.tables import *
-from .checks.nwbfile_metadata import *
 from .checks.ecephys import *

nwbinspector/checks/general.py

@@ -0,0 +1,23 @@
+"""Check functions that examine any general neurodata_type with the available attrbutes."""
+from ..register_checks import register_check, InspectorMessage, Importance
+
+COMMON_DESCRIPTION_PLACEHOLDERS = ["no description", "no desc", "none"]
+
+
+@register_check(importance=Importance.CRITICAL, neurodata_type=None)
+def check_name_slashes(obj):
+    """Check if there  has been added for the session."""
+    if hasattr(obj, "name") and any((x in obj.name for x in ["/", "\\"])):
+        return InspectorMessage(message="Object name contains slashes.")
+
+
+@register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=None)
+def check_description(obj):
+    """Check if the description is a not missing or a placeholder."""
+    common_description_placeholders = ["no description", "no desc", "none"]
+    if not hasattr(obj, "description"):
+        return
+    if obj.description is None or obj.description.strip(" ") == "":
+        return InspectorMessage(message="Description is missing.")
+    if obj.description.lower().strip(".") in common_description_placeholders:
+        return InspectorMessage(message=f"Description ('{obj.description}') is a placeholder.")

nwbinspector/checks/nwbfile_metadata.py

@@ -1,8 +1,8 @@
 """Check functions that examine general NWBFile metadata."""
 import re
 
-from pynwb import NWBFile
-from pynwb.file import Subject, ProcessingModule
+from pynwb import NWBFile, ProcessingModule
+from pynwb.file import Subject
 
 from ..register_checks import register_check, InspectorMessage, Importance
 
@@ -15,16 +15,6 @@
 PROCESSING_MODULE_CONFIG = ["ophys", "ecephys", "icephys", "behavior", "misc", "ogen", "retinotopy"]
 
 
-@register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=ProcessingModule)
-def check_processing_module_name(processing_module: ProcessingModule):
-    """Check if the name of a processing module is of a valid modality."""
-    if processing_module.name not in PROCESSING_MODULE_CONFIG:
-        return InspectorMessage(
-            f"Processing module is named {processing_module.name}. It is recommended to use the "
-            f"schema module names: {', '.join(PROCESSING_MODULE_CONFIG)}"
-        )
-
-
 @register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=NWBFile)
 def check_experimenter(nwbfile: NWBFile):
     """Check if an experimenter has been added for the session."""
@@ -115,3 +105,13 @@ def check_subject_species(subject: Subject):
         return InspectorMessage(
             message="Species should be in latin binomial form, e.g. 'Mus musculus' and 'Homo sapiens'",
         )
+
+
+@register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=ProcessingModule)
+def check_processing_module_name(processing_module: ProcessingModule):
+    """Check if the name of a processing module is of a valid modality."""
+    if processing_module.name not in PROCESSING_MODULE_CONFIG:
+        return InspectorMessage(
+            f"Processing module is named {processing_module.name}. It is recommended to use the "
+            f"schema module names: {', '.join(PROCESSING_MODULE_CONFIG)}"
+        )

nwbinspector/nwbinspector.py

@@ -378,7 +378,7 @@ def run_checks(nwbfile: pynwb.NWBFile, checks: list):
     """
     for check_function in checks:
         for nwbfile_object in nwbfile.objects.values():
-            if issubclass(type(nwbfile_object), check_function.neurodata_type):
+            if check_function.neurodata_type is None or issubclass(type(nwbfile_object), check_function.neurodata_type):
                 try:
                     output = check_function(nwbfile_object)
                 # if an individual check fails, include it in the report and continue with the inspection

nwbinspector/register_checks.py

@@ -78,7 +78,24 @@ class InspectorMessage:
 # TODO: neurodata_type could have annotation hdmf.utils.ExtenderMeta, which seems to apply to all currently checked
 # objects. We can wait and see how well that holds up before adding it in officially.
 def register_check(importance: Importance, neurodata_type):
-    """Wrap a check function to add it to the list of default checks for that severity and neurodata type."""
+    """
+    Wrap a check function with this decorator to add it to the check registry and automatically parse some output.
+
+    Parameters
+    ----------
+    importance : Importance
+        Importance has three levels:
+            CRITICAL
+                - potentially incorrect data
+            BEST_PRACTICE_VIOLATION
+                - very suboptimal data representation
+            BEST_PRACTICE_SUGGESTION
+                - improvable data representation
+    neurodata_type
+        The most generic HDMF/PyNWB class the check function applies to.
+        Should generally match the type annotation of the check.
+        If this check is intended to apply to any general NWBFile object, set neurodata_type to None.
+    """
 
     def register_check_and_auto_parse(check_function):
         if importance not in [

tests/unit_tests/test_general.py

@@ -0,0 +1,52 @@
+from hdmf.common import DynamicTable
+
+from nwbinspector import InspectorMessage, Importance
+from nwbinspector.checks.general import check_name_slashes, check_description
+
+
+def test_check_name_slashes_pass():
+    table = DynamicTable(name="test_name", description="")
+    assert check_name_slashes(obj=table) is None
+
+
+def test_check_name_slashes_fail():
+    """HDMF/PyNWB forbid "/" in the object names. Might need an external file written in MATLAB to test that?"""
+    for x in ["\\"]:
+        table = DynamicTable(name=f"test{x}ing", description="")
+        assert check_name_slashes(obj=table) == InspectorMessage(
+            message="Object name contains slashes.",
+            importance=Importance.CRITICAL,
+            check_function_name="check_name_slashes",
+            object_type="DynamicTable",
+            object_name=f"test{x}ing",
+            location="/",
+        )
+
+
+def test_check_description_pass():
+    table = DynamicTable(name="test", description="testing")
+    assert check_description(obj=table) is None
+
+
+def test_check_description_fail():
+    table = DynamicTable(name="test", description="No Description.")
+    assert check_description(obj=table) == InspectorMessage(
+        message="Description ('No Description.') is a placeholder.",
+        importance=Importance.BEST_PRACTICE_SUGGESTION,
+        check_function_name="check_description",
+        object_type="DynamicTable",
+        object_name="test",
+        location="/",
+    )
+
+
+def test_check_description_missing():
+    table = DynamicTable(name="test", description=" ")
+    assert check_description(obj=table) == InspectorMessage(
+        message="Description is missing.",
+        importance=Importance.BEST_PRACTICE_SUGGESTION,
+        check_function_name="check_description",
+        object_type="DynamicTable",
+        object_name="test",
+        location="/",
+    )

tests/unit_tests/test_nwbfile_metadata.py

@@ -1,8 +1,8 @@
 from uuid import uuid4
 from datetime import datetime
 
-from pynwb import NWBFile
-from pynwb.file import Subject, ProcessingModule
+from pynwb import NWBFile, ProcessingModule
+from pynwb.file import Subject
 import pytest
 
 from nwbinspector import InspectorMessage, Importance
@@ -20,7 +20,6 @@
     check_processing_module_name,
     PROCESSING_MODULE_CONFIG,
 )
-from nwbinspector.register_checks import Severity
 from nwbinspector.tools import make_minimal_nwbfile
 
 
@@ -278,6 +277,11 @@ def test_pass_check_subject_id_exist():
     assert check_subject_id_exists(subject) is None
 
 
+@pytest.mark.skip(reason="TODO")
+def test_check_subject_species():
+    pass
+
+
 def test_check_processing_module_name():
     processing_module = ProcessingModule("test", "desc")
     assert check_processing_module_name(processing_module) == InspectorMessage(
@@ -294,8 +298,3 @@ def test_check_processing_module_name():
 def test_pass_check_processing_module_name():
     processing_module = ProcessingModule("ecephys", "desc")
     assert check_processing_module_name(processing_module) is None
-
-
-@pytest.mark.skip(reason="TODO")
-def test_check_subject_species():
-    pass