Merge pull request #325 from catalystneuro/add_check_for_timestamps_are_not_negative

Add inspector check for negative timestamps

Author: CodyCBakerPhD

docs/best_practices/nwbfile_metadata.rst

@@ -22,13 +22,18 @@ the ``timestamp_reference_time`` used across all of the NWBFiles may be set sepa
 All time-related data in the NWBFile should be synchronized to the ``timestamps_reference_time`` so that future users
 are able to understand the timing of all events contained within the NWBFile.
 
+The ``timestamps_reference_time`` should also be the earliest timestamp in the file, giving all other time references
+a positive value relative to that. There should be no time references which are negative.
+
 Given the importance of this field within an :ref:`nwb-schema:sec-NWBFile`, is it critical that it be set to a proper
 value. Default values should generally not be used for this field. If the true date is unknown, use your
 best guess. If the exact start time is unknown, then it is fine to simply set it to midnight on that date.
 
 
 Check functions: :py:meth:`~nwbinspector.checks.nwbfile_metadata.check_session_start_time_old_date`,
 :py:meth:`~nwbinspector.checks.nwbfile_metadata.check_session_start_time_future_date`,
+:py:meth:`~nwbinspector.checks.time_series.check_timestamp_of_the_first_sample_is_not_negative`
+:py:meth:`~nwbinspector.checks.tables.check_table_time_columns_are_not_negative`
 
 
 

src/nwbinspector/checks/tables.py

@@ -235,3 +235,24 @@ def check_ids_unique(table: DynamicTable, nelems: Optional[int] = NELEMS):
     data = table.id[:nelems]
     if len(set(data)) != len(data):
         return InspectorMessage(message="This table has ids that are not unique.")
+
+
+@register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=DynamicTable)
+def check_table_time_columns_are_not_negative(table: DynamicTable):
+    """
+    Check that time columns are not negative.
+
+    Best Practice: :ref:`best_practice_global_time_reference`
+
+    Parameters
+    ----------
+    table: DynamicTable
+    """
+    for column_name in table.colnames:
+        if column_name.endswith("_time"):
+            first_timestamp = table[column_name][0]
+            if first_timestamp < 0:
+                yield InspectorMessage(
+                    message=f"Timestamps in column {column_name} should not be negative."
+                    " It is recommended to align the `session_start_time` or `timestamps_reference_time` to be the earliest time value that occurs in the data, and shift all other signals accordingly."
+                )

src/nwbinspector/checks/time_series.py

@@ -88,6 +88,22 @@ 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_SUGGESTION, neurodata_type=TimeSeries)
+def check_timestamp_of_the_first_sample_is_not_negative(time_series: TimeSeries):
+    """
+    Check that the timestamp of the first sample is not negative.
+
+    Best Practice: :ref:`best_practice_global_time_reference`
+    """
+
+    first_timestamp = time_series.starting_time if time_series.starting_time is not None else time_series.timestamps[0]
+    if first_timestamp < 0:
+        return InspectorMessage(
+            message="Timestamps should not be negative."
+            " It is recommended to align the `session_start_time` or `timestamps_reference_time` to be the earliest time value that occurs in the data, and shift all other signals accordingly."
+        )
+
+
 @register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=TimeSeries)
 def check_missing_unit(time_series: TimeSeries):
     """

tests/unit_tests/test_tables.py

@@ -19,6 +19,7 @@
     check_table_values_for_dict,
     check_col_not_nan,
     check_ids_unique,
+    check_table_time_columns_are_not_negative,
 )
 from nwbinspector.utils import get_package_version
 
@@ -416,3 +417,40 @@ def test_fail_check_ids_unique():
 def test_pass_check_ids_unique():
     dt = DynamicTable(name="test_table", description="test", id=[0, 1])
     assert check_ids_unique(dt) is None
+
+
+def test_table_time_columns_are_not_negative_fail():
+    test_table = DynamicTable(name="test_table", description="test")
+    test_table.add_column(name="test_time", description="")
+    test_table.add_column(name="start_time", description="")
+    test_table.add_column(name="stop_time", description="")
+    test_table.add_row(test_time=-2.0, start_time=-1.0, stop_time=3.0)
+
+    assert check_table_time_columns_are_not_negative(test_table) == [
+        InspectorMessage(
+            message=f"Timestamps in column test_time should not be negative."
+            " It is recommended to align the `session_start_time` or `timestamps_reference_time` to be the earliest time value that occurs in the data, and shift all other signals accordingly.",
+            importance=Importance.BEST_PRACTICE_SUGGESTION,
+            check_function_name="check_table_time_columns_are_not_negative",
+            object_type="DynamicTable",
+            object_name="test_table",
+            location="/",
+        ),
+        InspectorMessage(
+            message=f"Timestamps in column start_time should not be negative."
+            " It is recommended to align the `session_start_time` or `timestamps_reference_time` to be the earliest time value that occurs in the data, and shift all other signals accordingly.",
+            importance=Importance.BEST_PRACTICE_SUGGESTION,
+            check_function_name="check_table_time_columns_are_not_negative",
+            object_type="DynamicTable",
+            object_name="test_table",
+            location="/",
+        ),
+    ]
+
+
+def test_table_time_columns_are_not_negative_pass():
+    test_table = DynamicTable(name="test_table", description="test")
+    test_table.add_column(name="test_time", description="")
+    test_table.add_row(test_time=1.0)
+
+    assert check_table_time_columns_are_not_negative(test_table) is None

tests/unit_tests/test_time_series.py

@@ -14,6 +14,7 @@
     check_timestamps_ascending,
     check_missing_unit,
     check_resolution,
+    check_timestamp_of_the_first_sample_is_not_negative,
 )
 from nwbinspector.tools import make_minimal_nwbfile
 from nwbinspector.testing import check_streaming_tests_enabled
@@ -215,6 +216,54 @@ def test_check_timestamps_ascending_fail():
     )
 
 
+def test_check_timestamp_of_the_first_sample_is_not_negative_with_timestamps_fail():
+    time_series = pynwb.TimeSeries(name="test_time_series", unit="test_units", data=[1, 2, 3], timestamps=[-1, 0, 1])
+    message = (
+        "Timestamps should not be negative."
+        " It is recommended to align the `session_start_time` or `timestamps_reference_time` "
+        "to be the earliest time value that occurs in the data, and shift all other signals accordingly."
+    )
+    assert check_timestamp_of_the_first_sample_is_not_negative(time_series) == InspectorMessage(
+        message=message,
+        importance=Importance.BEST_PRACTICE_SUGGESTION,
+        check_function_name="check_timestamp_of_the_first_sample_is_not_negative",
+        object_type="TimeSeries",
+        object_name="test_time_series",
+        location="/",
+    )
+
+
+def test_check_timestamp_of_the_first_sample_is_not_negative_with_timestamps_pass():
+    time_series = pynwb.TimeSeries(name="test_time_series", unit="test_units", data=[1, 2, 3], timestamps=[0, 1, 2])
+    assert check_timestamp_of_the_first_sample_is_not_negative(time_series) is None
+
+
+def test_check_timestamp_of_the_first_sample_is_not_negative_with_starting_time_fail():
+    time_series = pynwb.TimeSeries(
+        name="test_time_series", unit="test_units", data=[1, 2, 3], starting_time=-1.0, rate=30.0
+    )
+    message = (
+        "Timestamps should not be negative."
+        " It is recommended to align the `session_start_time` or `timestamps_reference_time` "
+        "to be the earliest time value that occurs in the data, and shift all other signals accordingly."
+    )
+    assert check_timestamp_of_the_first_sample_is_not_negative(time_series) == InspectorMessage(
+        message=message,
+        importance=Importance.BEST_PRACTICE_SUGGESTION,
+        check_function_name="check_timestamp_of_the_first_sample_is_not_negative",
+        object_type="TimeSeries",
+        object_name="test_time_series",
+        location="/",
+    )
+
+
+def test_check_timestamp_of_the_first_sample_is_not_negative_with_starting_time_pass():
+    time_series = pynwb.TimeSeries(
+        name="test_time_series", unit="test_units", data=[1, 2, 3], starting_time=0.0, rate=30.0
+    )
+    assert check_timestamp_of_the_first_sample_is_not_negative(time_series) is None
+
+
 def test_check_missing_unit_pass():
     time_series = pynwb.TimeSeries(name="test_time_series", unit="test_units", data=[1, 2, 3], timestamps=[1, 2, 3])
     assert check_missing_unit(time_series) is None