Merge branch 'dev' into check_session_start_time

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`

docs/best_practices/time_series.rst

@@ -35,6 +35,8 @@ Every :nwb-schema:ref:`sec-TimeSeries` instance has ``unit`` as an attribute, wh
 measurement for that data, using the appropriate type from the
 :wikipedia:`International System of Units (SI) <International_System_of_Units>`.
 
+Check function: :py:meth:`~nwbinspector.checks.time_series.check_missing_unit`
+
 
 
 .. _best_practice_time_series_global_time_reference:

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

@@ -2,8 +2,8 @@
 import re
 from datetime import datetime
 
-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
 
@@ -16,16 +16,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_session_start_time_old_date(nwbfile: NWBFile):
     """Check if the session_start_time was set to an appropriate value."""
@@ -137,3 +127,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/checks/time_series.py

@@ -68,34 +68,23 @@ def check_timestamps_ascending(time_series: TimeSeries, nelems=200):
         return InspectorMessage(f"{time_series.name} timestamps are not ascending.")
 
 
+@register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=TimeSeries)
+def check_missing_unit(time_series: TimeSeries):
+    """
+    Check if the TimeSeries.unit field is empty.
+
+    Best Practice: :ref:`best_practice_unit_of_measurement`
+    """
+    if not time_series.unit:
+        return InspectorMessage(
+            message="Missing text for attribute 'unit'. Please specify the scientific unit of the 'data'."
+        )
+
+
 @register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=TimeSeries)
 def check_resolution(time_series: TimeSeries):
     """Check the resolution value of a TimeSeries for proper format (-1.0 or NaN for unknown)."""
     if time_series.resolution != -1.0 and time_series.resolution <= 0:
         return InspectorMessage(
             message=f"'resolution' should use -1.0 or NaN for unknown instead of {time_series.resolution}."
         )
-
-
-# TODO: break up logic of extra stuff into separate checks
-# def check_timeseries(nwbfile):
-#     """Check dataset values in TimeSeries objects"""
-#     for ts in all_of_type(nwbfile, pynwb.TimeSeries):
-#         if ts.data is None:
-#             # exception to the rule: ImageSeries objects are allowed to have no data
-#             if not isinstance(ts, pynwb.image.ImageSeries):
-#                 error_code = "A101"
-#                 print("- %s: '%s' %s data is None" % (error_code, ts.name, type(ts).__name__))
-#             else:
-#                 if ts.external_file is None:
-#                     error_code = "A101"
-#                     print(
-#                         "- %s: '%s' %s data is None and external_file is None"
-#                         % (error_code, ts.name, type(ts).__name__)
-#                     )
-#             continue
-
-#         if not ts.unit:
-#             error_code = "A101"
-#             print("- %s: '%s' %s data is missing text for attribute 'unit'"
-# % (error_code, ts.name, type(ts).__name__))

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="/",
+    )