Repository: NeurodataWithoutBorders/nwbinspector (language: python)

Issue:
[Bug]: nwbinspector does not support a duration of years
### What happened?

When entering human data, you generally want to report age in years. This is valid ISO 6801, but is not parsed by pandas. The reasoning, as explained [here](https://github.com/pandas-dev/pandas/issues/11375) is that months and years are not fixed intervals. They can be of variable length depending on the month or year. This is not the case with seconds, minutes, or hours.

So they have no interest in supporting this in pd.Timestamps, which we use to parse ISO 8601 durations [here](https://github.com/NeurodataWithoutBorders/nwbinspector/blob/99160f853eb74850e2bd9d3ac46d7c7796879235/src/nwbinspector/checks/nwbfile_metadata.py#L174).

The result is this:

```
(base) MacBook-Pro-8:000397 bendichter$ nwbinspector . --config dandi
                                                                                                                                                                                                           

**************************************************
NWBInspector Report Summary

Timestamp: 2022-11-29 13:25:18.636881-08:00
Platform: macOS-13.0-arm64-arm-64bit
NWBInspector version: 0.4.21

Found 27 issues over 3 files:
       3 - ERROR
      24 - BEST_PRACTICE_SUGGESTION
**************************************************


0  ERROR
========

0.0  sub-Pt01/sub-Pt01_ecephys.nwb and 2 other files: check_subject_proper_age_range - 'None' object with name 'None'
       Message: Traceback (most recent call last):
  File "/Users/bendichter/dev/nwbinspector/src/nwbinspector/nwbinspector.py", line 563, in run_checks
    output = robust_s3_read(command=check_function, command_args=[nwbfile_object])
  File "/Users/bendichter/dev/nwbinspector/src/nwbinspector/utils.py", line 171, in robust_s3_read
    raise exc
  File "/Users/bendichter/dev/nwbinspector/src/nwbinspector/utils.py", line 166, in robust_s3_read
    return command(*command_args, **command_kwargs)
  File "/Users/bendichter/dev/nwbinspector/src/nwbinspector/register_checks.py", line 127, in auto_parse_some_output
    output = check_function(*args, **kwargs)
  File "/Users/bendichter/dev/nwbinspector/src/nwbinspector/checks/nwbfile_metadata.py", line 174, in check_subject_proper_age_range
    and Timedelta(subject_lower_age_bound) >= Timedelta(subject_upper_age_bound)
  File "pandas/_libs/tslibs/timedeltas.pyx", line 1683, in pandas._libs.tslibs.timedeltas.Timedelta.__new__
  File "pandas/_libs/tslibs/timedeltas.pyx", line 922, in pandas._libs.tslibs.timedeltas.parse_iso_format_string
ValueError: Invalid ISO 8601 Duration format - P34Y
```

The [discussions in pandas](https://github.com/pandas-dev/pandas/pull/15136) point to [this library](https://pypi.org/project/isodate/), which handles all durations, including years and months.

### Steps to Reproduce

_No response_

### Traceback

_No response_

### Operating System

macOS

### Python Executable

Python

### Python Version

3.9

### Usage

Command Line Interface

### Were you streaming with ROS3?

No

### Package Versions

_No response_

### Code of Conduct

- [X] I agree to follow this project's [Code of Conduct](https://github.com/NeurodataWithoutBorders/nwbinspector/blob/dev/.github/CODE_OF_CONDUCT.rst)
- [X] Have you ensured this bug was not already [reported](https://github.com/neurodatawithoutborders/nwbinspector/issues)?
- [X] To the best of your ability, have you ensured this is a bug within the code that checks the NWBFile, rather than a bug in the NWBFile reader (e.g., [PyNWB](https://github.com/NeurodataWithoutBorders/pynwb) or [MatNWB](https://github.com/NeurodataWithoutBorders/matnwb))?


Discussion / hints:
isodate has the same problem:

https://github.com/gweis/isodate/blob/8856fdf0e46c7bca00229faa1aae6b7e8ad6e76c/src/isodate/isoduration.py#L30-L35


If the datestring contains months or years, it returns a native `Duration` object, which can't be compared with `>` or `<`
native `timedelta` objects don't support months or years either

https://docs.python.org/3/library/datetime.html#datetime.timedelta
Really surprising all these fancy packages have chosen not to allow comparison between two time deltas that have the same relative offset (and therefore whatever looseness comes from variability in months/years would be equivalent for the two time deltas). Sounds like we'll have to use that function you found from stack overflow that does it manually.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index be31b08e1..0af1270b5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Upcoming
 
+### Fixes
+* Fix `check_subject_proper_age_range` to parse years. [PR #314](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/314)
+
+
 # v0.4.20
 
 ### Improvements
@@ -15,7 +19,6 @@
 
 * Fixed issue in `run_checks` following [PR #303](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/303) that prevented iteration over certain check output types. [PR #306](https://github.com/NeurodataWithoutBorders/nwbinspector/pull/306)
 
-
 # v0.4.19
 
 ### Fixes
diff --git a/requirements.txt b/requirements.txt
index cc9cf7405..83b002106 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -5,5 +5,6 @@ packaging
 natsort
 click
 tqdm
+isodate
 numpy>=1.20.0,<1.21.0;python_version<'3.8'
 numpy>=1.22.0,<1.23.0;python_version>='3.8'  # <1.23 bound is from PyNWB
diff --git a/src/nwbinspector/checks/nwbfile_metadata.py b/src/nwbinspector/checks/nwbfile_metadata.py
index df99217a8..b13b6b4d0 100644
--- a/src/nwbinspector/checks/nwbfile_metadata.py
+++ b/src/nwbinspector/checks/nwbfile_metadata.py
@@ -2,7 +2,7 @@
 import re
 from datetime import datetime
 
-from pandas import Timedelta
+from isodate import parse_duration, Duration
 from pynwb import NWBFile, ProcessingModule
 from pynwb.file import Subject
 
@@ -168,17 +168,24 @@ def check_subject_proper_age_range(subject: Subject):
     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)
+        if re.fullmatch(pattern=duration_regex, string=subject_lower_age_bound) and re.fullmatch(
+            pattern=duration_regex, string=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."
+            lower = parse_duration(subject_lower_age_bound)
+            if isinstance(lower, Duration):
+                lower = lower.totimedelta(end=datetime.now())
+
+            upper = parse_duration(subject_upper_age_bound)
+            if isinstance(upper, Duration):
+                upper = upper.totimedelta(end=datetime.now())
+
+            if lower >= upper:
+                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)


Test patch:
diff --git a/tests/unit_tests/test_nwbfile_metadata.py b/tests/unit_tests/test_nwbfile_metadata.py
index bebc71ba0..a1ebf1b9d 100644
--- a/tests/unit_tests/test_nwbfile_metadata.py
+++ b/tests/unit_tests/test_nwbfile_metadata.py
@@ -394,6 +394,26 @@ def test_check_subject_proper_age_range_fail():
     )
 
 
+def test_check_subject_age_with_years():
+    subject = Subject(subject_id="001", age="P1Y/P2Y")
+    assert check_subject_age(subject) is None
+
+
+def test_check_subject_age_with_years_fail():
+    subject = Subject(subject_id="001", age="P2Y/P1Y")
+    assert check_subject_proper_age_range(subject) == InspectorMessage(
+        message=(
+            "The durations of the Subject age range, 'P2Y/P1Y', 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