Repository: NeurodataWithoutBorders/nwbinspector (language: python)

Issue:
[Add Check]: Check that if experimenters string does not have comma
If a user provides a comma-separated scalar string for `nwbfile.experimeter`, the inspector should recommend that they instead store an array of experimenter names. Same for keywords and related publications.


Discussion / hints:
This is a bit tricky for experimenters, because they might have listed the name Last, First
The [BP section for experimenters](https://nwbinspector.readthedocs.io/en/dev/best_practices/nwbfile_metadata.html#experimenter) does not describe the format in which they ought to write these things down: should this be updated to some standard, such as 

Last Name, M. I., First Name 

M. I. - 'middle initial'

And how should multiple experimenters be handled?
multiple experimenters should be handled as a vector of strings
OK so, as I understand the heart of this original issue is to try to detect people encoding multiple experimenters with comma-separation instead of being added as different elements of the vector.

However, there are several different ways people can encode a single experimenter name, such as

(a) Firstname Lastname
(b) Lastname, Firstname
(c) [Optional]: Firstname, M.I., Lastname
(di) [Optional]: Lastname, Firstname, M.I.
(dii) [Optional]: Lastname, Firstname M.I.
(e) [Optional]: Lastname, M.I., Firstname

Now, we can still try to catch the heart of this issue by triggering the violation `if any((experimenter.count(",") for experimenter in nwbfile.experimenters)) > 3`

However, since the Best Practices make many references to trying to make NWBFiles machine-readable, I wonder if it might be in our best interest to encourage a single standard for encoding names (from a-e above), which would also make this kind of check easier and more reliable to catch.

Note: the [experimenter schema](https://nwb-schema.readthedocs.io/en/latest/format.html?highlight=experimenter#groups-general) also mentions that roles can be specified. I'd propose that a Best Practice for that could be optionally including `NameOfRole: ` before the name of the individual to make this easier to parse.
Here's DANDI's regex for name: https://github.com/dandi/dandi-schema/blob/ffa49ef5dc84bc5a234fbc9ca9ba04b872539893/dandischema/models.py#L48
> Here's DANDI's regex for name: https://github.com/dandi/dandi-schema/blob/ffa49ef5dc84bc5a234fbc9ca9ba04b872539893/dandischema/models.py#L48

Then their regex matches option (dii).

That could regex could be improved however; note that it allows numbers included in names (try `re.fullmatch(pattern=r"^([\w\s\-]+),\s+([\w\s\-\.]+)$", string="Baker2, Cody1")`) and disallows apostrophes (like`re.fullmatch(pattern=r"^([\w\s\-]+),\s+([\w\s\-\.]+)$", string="O'Baker, Cody")`).

Also note it does not accommodate role assignment of each experimenter, which the [NWB Schema](https://nwb-schema.readthedocs.io/en/latest/format.html?highlight=experimenter#groups-general) indicates is allowed

@bendichter So do you think we should make it a Best Practice to enforce structure (dii)? How should we deal with roles?
@CodyCBakerPhD good points. Maybe we should raise these issues on the dandi schema repo
Turns out that was an older version (not most recent master): https://github.com/dandi/dandi-schema/blob/master/dandischema/models.py#L60

And they may continue to think numbers in names are OK.

One thing to note though as I look through their code and how they use it, I'm not 100% sure they actually require the NWBAsset experimenter metadata to follow that, that's just what they enforce for their dandi assets after extraction. So whatever format the NWB team decides to support for this as a 'best practice' can always be mapped into what dandi expects via their intermediate metadata handling functions.
@CodyCBakerPhD yes, exactly. This is a dandiset metadata requirement. I'd like to have the option of automatically pulling the experimenter metadata into the dandiset metadata, which is why I thought it might be convenient to have the same regex as a best practice.

Fix patch:
diff --git a/nwbinspector/checks/nwbfile_metadata.py b/nwbinspector/checks/nwbfile_metadata.py
index 229b5d451..096e488aa 100644
--- a/nwbinspector/checks/nwbfile_metadata.py
+++ b/nwbinspector/checks/nwbfile_metadata.py
@@ -6,6 +6,7 @@
 from pynwb.file import Subject
 
 from ..register_checks import register_check, InspectorMessage, Importance
+from ..utils import is_module_installed
 
 duration_regex = (
     r"^P(?!$)(\d+(?:\.\d+)?Y)?(\d+(?:\.\d+)?M)?(\d+(?:\.\d+)?W)?(\d+(?:\.\d+)?D)?(T(?=\d)(\d+(?:\.\d+)?H)?(\d+(?:\.\d+)"
@@ -38,12 +39,31 @@ def check_session_start_time_future_date(nwbfile: NWBFile):
 
 
 @register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=NWBFile)
-def check_experimenter(nwbfile: NWBFile):
+def check_experimenter_exists(nwbfile: NWBFile):
     """Check if an experimenter has been added for the session."""
     if not nwbfile.experimenter:
         return InspectorMessage(message="Experimenter is missing.")
 
 
+@register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=NWBFile)
+def check_experimenter_form(nwbfile: NWBFile):
+    """Check the text form of each experimenter to see if it matches the DANDI regex pattern."""
+    if is_module_installed(module_name="dandi"):
+        from dandischema.models import NAME_PATTERN  # for most up to date version of the regex
+    else:
+        NAME_PATTERN = r"^([\w\s\-\.']+),\s+([\w\s\-\.']+)$"  # copied on 7/12/22
+
+    for experimenter in nwbfile.experimenter:
+        experimenter = experimenter.decode() if isinstance(experimenter, bytes) else experimenter
+        if re.match(string=experimenter, pattern=NAME_PATTERN) is None:
+            yield InspectorMessage(
+                message=(
+                    f"The name of experimenter '{experimenter}' does not match the DANDI form "
+                    "(Last, First Middle or Last, First M.)."
+                )
+            )
+
+
 @register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=NWBFile)
 def check_experiment_description(nwbfile: NWBFile):
     """Check if a description has been added for the session."""


Test patch:
diff --git a/tests/unit_tests/test_nwbfile_metadata.py b/tests/unit_tests/test_nwbfile_metadata.py
index ec06c38cc..7b9acdc17 100644
--- a/tests/unit_tests/test_nwbfile_metadata.py
+++ b/tests/unit_tests/test_nwbfile_metadata.py
@@ -7,7 +7,8 @@
 from nwbinspector import (
     InspectorMessage,
     Importance,
-    check_experimenter,
+    check_experimenter_exists,
+    check_experimenter_form,
     check_experiment_description,
     check_institution,
     check_keywords,
@@ -70,37 +71,79 @@ def test_check_session_start_time_future_date_fail():
     )
 
 
-def test_check_experimenter_pass():
+def test_check_experimenter_exists_pass():
     nwbfile = NWBFile(
         session_description="",
         identifier=str(uuid4()),
         session_start_time=datetime.now().astimezone(),
         experimenter=["Last, First"],
     )
-    assert check_experimenter(nwbfile) is None
+    assert check_experimenter_exists(nwbfile) is None
 
 
-def test_check_experimenter_bytestring_pass():
+def test_check_experimenter_exists_bytestring_pass():
     nwbfile = NWBFile(
         session_description="",
         identifier=str(uuid4()),
         session_start_time=datetime.now().astimezone(),
         experimenter=[b"Last, First"],
     )
-    assert check_experimenter(nwbfile) is None
+    assert check_experimenter_exists(nwbfile) is None
 
 
-def test_check_experimenter_fail():
-    assert check_experimenter(minimal_nwbfile) == InspectorMessage(
+def test_check_experimenter_exists_fail():
+    assert check_experimenter_exists(minimal_nwbfile) == InspectorMessage(
         message="Experimenter is missing.",
         importance=Importance.BEST_PRACTICE_SUGGESTION,
-        check_function_name="check_experimenter",
+        check_function_name="check_experimenter_exists",
         object_type="NWBFile",
         object_name="root",
         location="/",
     )
 
 
+def test_check_experimenter_form_pass():
+    nwbfile = NWBFile(
+        session_description="",
+        identifier=str(uuid4()),
+        session_start_time=datetime.now().astimezone(),
+        experimenter=["Last, First"],
+    )
+    assert check_experimenter_form(nwbfile=nwbfile) is None
+
+
+def test_check_experimenter_form_bytestring_pass():
+    nwbfile = NWBFile(
+        session_description="",
+        identifier=str(uuid4()),
+        session_start_time=datetime.now().astimezone(),
+        experimenter=[b"Last, First"],
+    )
+    assert check_experimenter_form(nwbfile=nwbfile) is None
+
+
+def test_check_experimenter_form_fail():
+    nwbfile = NWBFile(
+        session_description="",
+        identifier=str(uuid4()),
+        session_start_time=datetime.now().astimezone(),
+        experimenter=["First Middle Last"],
+    )
+    assert check_experimenter_form(nwbfile=nwbfile) == [
+        InspectorMessage(
+            message=(
+                "The name of experimenter 'First Middle Last' does not match the DANDI form "
+                "(Last, First Middle or Last, First M.)."
+            ),
+            importance=Importance.BEST_PRACTICE_SUGGESTION,
+            check_function_name="check_experimenter_form",
+            object_type="NWBFile",
+            object_name="root",
+            location="/",
+        )
+    ]
+
+
 def test_check_experiment_description():
     assert check_experiment_description(minimal_nwbfile) == InspectorMessage(
         message="Experiment description is missing.",