Update dandi.inspector_config.yaml (#247)

* Update dandi.inspector_config.yaml * enchanged * fix tests * remove unused lines * separate logic; cleanup * fix tests * Update nwbinspector/checks/nwbfile_metadata.py Co-authored-by: Ben Dichter <[email protected]> * Update nwbinspector/checks/nwbfile_metadata.py Co-authored-by: Ben Dichter <[email protected]> * Update tests/unit_tests/test_nwbfile_metadata.py Co-authored-by: Ben Dichter <[email protected]> * Update tests/unit_tests/test_nwbfile_metadata.py Co-authored-by: Ben Dichter <[email protected]> * update tests * Update tests/unit_tests/test_nwbfile_metadata.py Co-authored-by: Ben Dichter <[email protected]> * Update tests/unit_tests/test_nwbfile_metadata.py Co-authored-by: Ben Dichter <[email protected]> * remove unused fields * propose changes to best practices (#252) * propose changes to best practices * Update docs/best_practices/nwbfile_metadata.rst Co-authored-by: Cody Baker <[email protected]> * Update docs/best_practices/nwbfile_metadata.rst Co-authored-by: Cody Baker <[email protected]> Co-authored-by: Cody Baker <[email protected]> * Include check_subject_proper_age_range to DANDI config (#255) * Update dandi.inspector_config.yaml * Update test_check_configuration.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Cody Baker <[email protected]> Co-authored-by: Cody Baker <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
NeurodataWithoutBorders · Aug 24, 2022 · 2cbb997 · 2cbb997
1 parent c55b0dc
commit 2cbb997
Show file tree

Hide file tree

Showing 6 changed files with 149 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,14 @@
 # Upcoming
 
+### DANDI Configuration
+* `check_subject_sex`, `check_subject_species`, `check_subject_age`, `check_subject_proper_age_range` are now elevated to `CRITICAL` importance when using the "DANDI" configuration. Therefore, these are now required for passing `dandi validate`.
+
 ### Improvements
 * Enhanced human-readability of the return message from `check_experimenter_form`. [PR #254](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/254)
-* Add check for existence of ``IntracellularElectrode.cell_id`` [PR #256](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/256)
-* Add check for shape consistency between ``reference_images`` and the x, y, (z) dimensions of the ``image_mask`` of ``PlaneSegmentation``objects. [PR #257](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/257)
+* Extended check for ``Subject.age`` field with estimated age range using '/' separator [PR #247](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/247)
+
+### New Checks
+* Added check for existence of ``IntracellularElectrode.cell_id`` [PR #256](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/256)
+* Added check that bounds of age range for ``Subject.age`` using the '/' separator are properly increasing. [PR #247](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/247)
+* Added check for existence of ``IntracellularElectrode.cell_id`` [PR #256](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/256)
+* Added check for shape consistency between ``reference_images`` and the x, y, (z) dimensions of the ``image_mask`` of ``PlaneSegmentation``objects. [PR #257](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/257)
diff --git a/docs/best_practices/nwbfile_metadata.rst b/docs/best_practices/nwbfile_metadata.rst
@@ -222,7 +222,11 @@ Age
 
 The ``age`` of a :nwb-schema:ref:`sec-Subject` should use the :wikipedia:`ISO 8601 Duration <ISO_8601#Durations>`
 format. For instance indicating an age of 90 days would be 'P90D'. It is not necessary to include both ``age`` and
-``date_of_birth``, but at least one of them is recommended.
+``date_of_birth``, but at least one of them is required by the DANDI Archive and recommended in general.
+
+If the precise age is unknown, an age range can be given by "[lower bound]/[upper bound]" e.g. "P10D/P20D" would mean
+that the age is in between 10 and 20 days. If only the lower bound is known, then including only the slash after that lower bound can be used to indicate a
+missing bound. For instance, "P90Y/" would indicate that the age is 90 years or older.
 
 Check function: :py:meth:`~nwbinspector.checks.nwbfile_metadata.check_subject_age`
 

diff --git a/nwbinspector/checks/nwbfile_metadata.py b/nwbinspector/checks/nwbfile_metadata.py
@@ -2,6 +2,7 @@
 import re
 from datetime import datetime
 
+from pandas import Timedelta
 from pynwb import NWBFile, ProcessingModule
 from pynwb.file import Subject
 
@@ -114,17 +115,50 @@ def check_doi_publications(nwbfile: NWBFile):
 
 @register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=Subject)
 def check_subject_age(subject: Subject):
-    """Check if the Subject age is in ISO 8601."""
+    """Check if the Subject age is in ISO 8601 or our extension of it for ranges."""
     if subject.age is None:
         if subject.date_of_birth is None:
             return InspectorMessage(message="Subject is missing age and date_of_birth.")
-    elif not re.fullmatch(duration_regex, subject.age):
-        return InspectorMessage(
-            message=(
-                f"Subject age, '{subject.age}', does not follow ISO 8601 duration format, e.g. 'P2Y' for 2 years "
-                "or 'P23W' for 23 weeks."
-            )
+        else:
+            return
+    if re.fullmatch(pattern=duration_regex, string=subject.age):
+        return
+
+    if "/" in subject.age:
+        subject_lower_age_bound, subject_upper_age_bound = subject.age.split("/")
+
+        if re.fullmatch(pattern=duration_regex, string=subject_lower_age_bound) and (
+            re.fullmatch(pattern=duration_regex, string=subject_upper_age_bound) or subject_upper_age_bound == ""
+        ):
+            return
+
+    return InspectorMessage(
+        message=(
+            f"Subject age, '{subject.age}', does not follow ISO 8601 duration format, e.g. 'P2Y' for 2 years "
+            "or 'P23W' for 23 weeks. You may also specify a range using a '/' separator, e.g., 'P1D/P3D' for an "
+            "age range somewhere from 1 to 3 days. If you cannot specify the upper bound of the range, "
+            "you may leave the right side blank, e.g., 'P90Y/' means 90 years old or older."
         )
+    )
+
+
+@register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=Subject)
+def check_subject_proper_age_range(subject: Subject):
+    """Check if the Subject age, if specified as duration range (e.g., 'P1D/P3D'), has properly increasing bounds."""
+    if subject.age is not None and "/" in subject.age:
+        subject_lower_age_bound, subject_upper_age_bound = subject.age.split("/")
+
+        if (
+            re.fullmatch(pattern=duration_regex, string=subject_lower_age_bound)
+            and re.fullmatch(pattern=duration_regex, string=subject_upper_age_bound)
+            and Timedelta(subject_lower_age_bound) >= Timedelta(subject_upper_age_bound)
+        ):
+            return InspectorMessage(
+                message=(
+                    f"The durations of the Subject age range, '{subject.age}', are not strictly increasing. "
+                    "The upper (right) bound should be a longer duration than the lower (left) bound."
+                )
+            )
 
 
 @register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=Subject)

diff --git a/nwbinspector/internal_configs/dandi.inspector_config.yaml b/nwbinspector/internal_configs/dandi.inspector_config.yaml
@@ -1,8 +1,9 @@
 CRITICAL:  # All the fields under CRITICAL will be required for dandi validate to pass
   - check_subject_exists
   - check_subject_id_exists
+  - check_subject_sex
+  - check_subject_species
+  - check_subject_age
+  - check_subject_proper_age_range
 BEST_PRACTICE_VIOLATION:
-  - 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
diff --git a/tests/test_check_configuration.py b/tests/test_check_configuration.py
@@ -70,11 +70,15 @@ def test_load_config(self):
         self.assertDictEqual(
             d1=config,
             d2=dict(
-                CRITICAL=["check_subject_exists", "check_subject_id_exists"],
-                BEST_PRACTICE_VIOLATION=[
+                CRITICAL=[
+                    "check_subject_exists",
+                    "check_subject_id_exists",
                     "check_subject_sex",
                     "check_subject_species",
                     "check_subject_age",
+                    "check_subject_proper_age_range",
+                ],
+                BEST_PRACTICE_VIOLATION=[
                     "check_data_orientation",
                 ],
             ),

diff --git a/tests/unit_tests/test_nwbfile_metadata.py b/tests/unit_tests/test_nwbfile_metadata.py
@@ -17,6 +17,7 @@
     check_subject_id_exists,
     check_subject_sex,
     check_subject_age,
+    check_subject_proper_age_range,
     check_subject_species_exists,
     check_subject_species_latin_binomial,
     check_processing_module_name,
@@ -278,7 +279,7 @@ def test_check_subject_sex():
 
 
 def test_check_subject_sex_other_value():
-    subject = Subject(subject_id="001", sex="Male")
+    subject = Subject(subject_id="001", sex="Female")
 
     assert check_subject_sex(subject) == InspectorMessage(
         message="Subject.sex should be one of: 'M' (male), 'F' (female), 'O' (other), or 'U' (unknown).",
@@ -290,8 +291,13 @@ def test_check_subject_sex_other_value():
     )
 
 
+def test_pass_check_subject_age_with_dob():
+    subject = Subject(subject_id="001", sex="F", date_of_birth=datetime.now())
+    assert check_subject_age(subject) is None
+
+
 def test_check_subject_age_missing():
-    subject = Subject(subject_id="001", sex="Male")
+    subject = Subject(subject_id="001")
     assert check_subject_age(subject) == InspectorMessage(
         message="Subject is missing age and date_of_birth.",
         importance=Importance.BEST_PRACTICE_SUGGESTION,
@@ -302,12 +308,19 @@ def test_check_subject_age_missing():
     )
 
 
-def test_check_subject_age_iso8601():
-    subject = Subject(subject_id="001", sex="Male", age="9 months")
+def test_check_subject_age_iso8601_pass():
+    subject = Subject(subject_id="001", age="P1D")
+    assert check_subject_age(subject) is None
+
+
+def test_check_subject_age_iso8601_fail():
+    subject = Subject(subject_id="001", age="9 months")
     assert check_subject_age(subject) == InspectorMessage(
         message=(
-            "Subject age, '9 months', does not follow ISO 8601 duration format, e.g. 'P2Y' for 2 years or 'P23W' "
-            "for 23 weeks."
+            "Subject age, '9 months', does not follow ISO 8601 duration format, e.g. 'P2Y' for 2 years "
+            "or 'P23W' for 23 weeks. You may also specify a range using a '/' separator, e.g., 'P1D/P3D' for an "
+            "age range somewhere from 1 to 3 days. If you cannot specify the upper bound of the range, "
+            "you may leave the right side blank, e.g., 'P90Y/' means 90 years old or older."
         ),
         importance=Importance.BEST_PRACTICE_SUGGESTION,
         check_function_name="check_subject_age",
@@ -317,11 +330,70 @@ def test_check_subject_age_iso8601():
     )
 
 
-def test_pass_check_subject_age_with_dob():
-    subject = Subject(subject_id="001", sex="Male", date_of_birth=datetime.now())
+def test_check_subject_age_iso8601_range_pass_1():
+    subject = Subject(subject_id="001", age="P1D/P3D")
+    assert check_subject_age(subject) is None
+
+
+def test_check_subject_age_iso8601_range_pass_2():
+    subject = Subject(subject_id="001", age="P1D/")
     assert check_subject_age(subject) is None
 
 
+def test_check_subject_age_iso8601_range_fail_1():
+    subject = Subject(subject_id="001", age="9 months/12 months")
+    assert check_subject_age(subject) == InspectorMessage(
+        message=(
+            "Subject age, '9 months/12 months', does not follow ISO 8601 duration format, e.g. 'P2Y' for 2 years "
+            "or 'P23W' for 23 weeks. You may also specify a range using a '/' separator, e.g., 'P1D/P3D' for an "
+            "age range somewhere from 1 to 3 days. If you cannot specify the upper bound of the range, "
+            "you may leave the right side blank, e.g., 'P90Y/' means 90 years old or older."
+        ),
+        importance=Importance.BEST_PRACTICE_SUGGESTION,
+        check_function_name="check_subject_age",
+        object_type="Subject",
+        object_name="subject",
+        location="/general/subject",
+    )
+
+
+def test_check_subject_age_iso8601_range_fail_2():
+    subject = Subject(subject_id="001", age="9 months/")
+    assert check_subject_age(subject) == InspectorMessage(
+        message=(
+            "Subject age, '9 months/', does not follow ISO 8601 duration format, e.g. 'P2Y' for 2 years "
+            "or 'P23W' for 23 weeks. You may also specify a range using a '/' separator, e.g., 'P1D/P3D' for an "
+            "age range somewhere from 1 to 3 days. If you cannot specify the upper bound of the range, "
+            "you may leave the right side blank, e.g., 'P90Y/' means 90 years old or older."
+        ),
+        importance=Importance.BEST_PRACTICE_SUGGESTION,
+        check_function_name="check_subject_age",
+        object_type="Subject",
+        object_name="subject",
+        location="/general/subject",
+    )
+
+
+def test_check_subject_proper_age_range_pass():
+    subject = Subject(subject_id="001", age="P1D/P3D")
+    assert check_subject_proper_age_range(subject) is None
+
+
+def test_check_subject_proper_age_range_fail():
+    subject = Subject(subject_id="001", age="P3D/P1D")
+    assert check_subject_proper_age_range(subject) == InspectorMessage(
+        message=(
+            "The durations of the Subject age range, 'P3D/P1D', are not strictly increasing. "
+            "The upper (right) bound should be a longer duration than the lower (left) bound."
+        ),
+        importance=Importance.BEST_PRACTICE_SUGGESTION,
+        check_function_name="check_subject_proper_age_range",
+        object_type="Subject",
+        object_name="subject",
+        location="/general/subject",
+    )
+
+
 def test_pass_check_subject_species_exists():
     subject = Subject(subject_id="001", species="Homo sapiens")
     assert check_subject_species_exists(subject) is None
@@ -358,7 +430,7 @@ def test_check_subject_species_not_binomial():
 
 
 def test_pass_check_subject_age():
-    subject = Subject(subject_id="001", sex="Male", age="P9M")
+    subject = Subject(subject_id="001", age="P9M")
     assert check_subject_age(subject) is None
 
 
@@ -375,12 +447,12 @@ def test_check_subject_exists():
 
 def test_pass_check_subject_exists():
     nwbfile = NWBFile(session_description="", identifier=str(uuid4()), session_start_time=datetime.now().astimezone())
-    nwbfile.subject = Subject(subject_id="001", sex="Male")
+    nwbfile.subject = Subject(subject_id="001")
     assert check_subject_exists(nwbfile) is None
 
 
 def test_check_subject_id_exists():
-    subject = Subject(sex="Male")
+    subject = Subject(sex="F")
     assert check_subject_id_exists(subject) == InspectorMessage(
         message="subject_id is missing.",
         importance=Importance.BEST_PRACTICE_SUGGESTION,