Enhance error notifications

src/nwb2bids/_command_line_interface/_main.py

@@ -1,11 +1,12 @@
+import collections
 import pathlib
 import typing
 
 import rich_click
 
 from .._converters._run_config import RunConfig
 from .._core._convert_nwb_dataset import convert_nwb_dataset
-from .._inspection._inspection_result import Severity
+from .._inspection._inspection_result import InspectionResult, Severity
 from .._tools._pluralize import _pluralize
 from ..testing import generate_ephys_tutorial
 
@@ -112,23 +113,64 @@ def _run_convert_nwb_dataset(
 
     converter = convert_nwb_dataset(nwb_paths=handled_nwb_paths, run_config=run_config)
 
+    if silent:
+        return
+
     notifications = converter.messages
-    console_notification = ""
-    if notifications:
-        notification_text = (
-            f'\n{(n:=len(notifications))} {_pluralize(n=n, word="suggestion")} for improvement '
-            f'{_pluralize(n=n, word="was", plural="were")} found during conversion.'
+    notifications_by_severity: dict[Severity, list[InspectionResult]] = collections.defaultdict(list)
+    for notification in notifications:
+        notifications_by_severity[notification.severity].append(notification)
+    errors = notifications_by_severity[Severity.ERROR]
+    criticals = notifications_by_severity[Severity.CRITICAL]
+
+    if errors:
+        number_of_errors = len(errors)
+
+        top_three = errors[:3]
+        number_to_print = len(top_three)
+
+        text = (
+            "\nBIDS dataset was not successfully created!\n"
+            f'{_pluralize(n=number_to_print, phrase="An error was", plural="Some errors were")} '
+            "encountered during conversion.\n"
+        )
+        error_text = "".join(f"\n\t- {error.reason}" for error in top_three)
+        if number_to_print > 1 and number_of_errors > 3:
+            counting_text = f"The first {number_to_print} of {number_of_errors} are shown below:"
+        elif number_to_print >= 2:
+            counting_text = f"The first {number_to_print} are shown below:"
+        else:
+            counting_text = "The error is shown below:"
+        text += (
+            f"{counting_text}\n\n"
+            f"{error_text}\n\n"
+            # TODO: "The full log file can be found at {run_config.log_file_path}\n"
         )
-        console_notification += rich_click.style(text=notification_text, fg="yellow")
 
-    if console_notification != "" and not silent:
+        console_notification = rich_click.style(text=text, fg="red")
         rich_click.echo(message=console_notification)
+        return
 
-    not_any_failures = not any(notification.severity == Severity.ERROR for notification in notifications)
-    if not_any_failures and not silent:
-        text = "\nBIDS dataset was successfully created!\n\n"
-        console_notification = rich_click.style(text=text, fg="green")
+    if criticals:
+        text = (
+            "\nBIDS dataset was successfully created, but may not be valid!\n"
+            # TODO: "Please review the full notifications report at {run_config.log_file_path}\n\n"
+        )
+        console_notification = rich_click.style(text=text, fg="yellow")
         rich_click.echo(message=console_notification)
+        return
+
+    text = "\nBIDS dataset was successfully created!"
+    if notifications:
+        number_of_notifications = len(notifications)
+
+        text += (
+            f'\n{number_of_notifications} {_pluralize(n=number_of_notifications, phrase="suggestion")} for improvement '
+            f'{_pluralize(n=number_of_notifications, phrase="was", plural="were")} found during conversion.'
+            # TODO: " Please review the full notifications report at {run_config.log_file_path}\n"
+        )
+    console_notification = rich_click.style(text=text + "\n\n", fg="green")
+    rich_click.echo(message=console_notification)
 
 
 # nwb2bids tutorial

src/nwb2bids/_tools/_pluralize.py

@@ -1,7 +1,7 @@
-def _pluralize(n: int, word: str, plural: str | None = None) -> str:
+def _pluralize(n: int, phrase: str, plural: str | None = None) -> str:
     if n == 1:
-        return word
+        return phrase
     elif plural is None:
-        return word + "s"
+        return phrase + "s"
     else:
         return plural

src/nwb2bids/bids_models/_bids_session_metadata.py

@@ -35,9 +35,9 @@ class BidsSessionMetadata(BaseMetadataContainerModel):
     @property
     def messages(self) -> list[InspectionResult]:
         messages = self.participant.messages.copy()
-        messages += self._internal_messages
+        messages += self._internal_messages.copy()
         if self.events is not None:
-            messages += self.events.messages
+            messages += self.events.messages.copy()
         if self.probe_table is not None:
             messages += self.probe_table.messages
         if self.channel_table is not None:

src/nwb2bids/bids_models/_probes.py

@@ -1,12 +1,13 @@
 import json
 import pathlib
+from typing import Any
 
 import pandas
 import pydantic
 import pynwb
 import typing_extensions
 
-from .._inspection._inspection_result import InspectionResult
+from .._inspection._inspection_result import Category, DataStandard, InspectionResult, Severity
 from ..bids_models._base_metadata_model import BaseMetadataContainerModel, BaseMetadataModel
 
 
@@ -20,6 +21,28 @@ class Probe(BaseMetadataModel):
 class ProbeTable(BaseMetadataContainerModel):
     probes: list[Probe]
 
+    def _check_fields(self) -> None:
+        # Check if values are specified
+        self._internal_messages = []
+
+        probes_missing_description = [probe for probe in self.probes if probe.description is None]
+        for probe_missing_description in probes_missing_description:
+            self._internal_messages.append(
+                InspectionResult(
+                    title="Missing description",
+                    reason="A basic description of this field is recommended to improve contextual understanding.",
+                    solution="Add a description to the field.",
+                    field=f"nwbfile.devices.{probe_missing_description.probe_id}",
+                    source_file_paths=[],  # TODO: figure out better way of handling
+                    data_standards=[DataStandard.BIDS, DataStandard.NWB],
+                    category=Category.STYLE_SUGGESTION,
+                    severity=Severity.INFO,
+                )
+            )
+
+    def model_post_init(self, context: Any, /) -> None:
+        self._check_fields()
+
     @pydantic.computed_field
     @property
     def messages(self) -> list[InspectionResult]:
@@ -29,19 +52,15 @@ def messages(self) -> list[InspectionResult]:
         These can accumulate over time based on which instance methods have been called.
         """
         messages = [message for probe in self.probes for message in probe.messages]
+        messages += self._internal_messages
         messages.sort(key=lambda message: (-message.category.value, -message.severity.value, message.title))
         return messages
 
     @classmethod
     @pydantic.validate_call
     def from_nwbfiles(cls, nwbfiles: list[pydantic.InstanceOf[pynwb.NWBFile]]) -> typing_extensions.Self | None:
-        electrical_series = [
-            neurodata_object
-            for nwbfile in nwbfiles
-            for neurodata_object in nwbfile.objects.values()
-            if isinstance(neurodata_object, pynwb.ecephys.ElectricalSeries)
-        ]
-        if any(electrical_series) is False:
+        nwb_electrode_tables = [nwbfile.electrodes for nwbfile in nwbfiles]
+        if not any(nwb_electrode_tables):
             return None
 
         unique_devices = {

tests/conftest.py

@@ -4,6 +4,7 @@
 import pathlib
 import shutil
 import sys
+import uuid
 
 import py.path
 import pynwb
@@ -335,6 +336,37 @@ def problematic_nwbfile_path_3(testing_files_directory: pathlib.Path) -> pathlib
     return nwbfile_path
 
 
+@pytest.fixture(scope="session")
+def problematic_nwbfile_path_4(testing_files_directory: pathlib.Path) -> pathlib.Path:
+    """
+    A fourth NWB file with less problematic metadata corresponding only to low-level 'info' events.
+    """
+    nwbfile = pynwb.NWBFile(
+        identifier=str(uuid.uuid4()),
+        session_id="problematic4",
+        session_description="",
+        session_start_time=datetime.datetime.now().astimezone(),
+    )
+    subject = pynwb.file.Subject(
+        subject_id="123",
+        species="Mus musculus",
+        sex="M",
+    )
+    nwbfile.subject = subject
+
+    device = pynwb.testing.mock.ecephys.mock_Device(name="DeviceWithoutDescription", description=None, nwbfile=nwbfile)
+    group = pynwb.testing.mock.ecephys.mock_ElectrodeGroup(device=device, nwbfile=nwbfile)
+    nwbfile.add_electrode(group=group, location="unknown")
+
+    problematic_subdirectory = testing_files_directory / "problematic"
+    problematic_subdirectory.mkdir(exist_ok=True)
+    nwbfile_path = problematic_subdirectory / "problematic4.nwb"
+    with pynwb.NWBHDF5IO(path=nwbfile_path, mode="w") as file_stream:
+        file_stream.write(nwbfile)
+
+    return nwbfile_path
+
+
 @pytest.fixture(scope="session")
 def problematic_nwbfile_path_missing_session_id(testing_files_directory: pathlib.Path) -> pathlib.Path:
     nwbfile = pynwb.testing.mock.file.mock_NWBFile(session_id=None)

tests/convert_nwb_dataset/test_cli_messages.py

@@ -4,12 +4,74 @@
 import subprocess
 
 
-def test_problematic_cli_messages(problematic_nwbfile_path_1: pathlib.Path, temporary_bids_directory: pathlib.Path):
+def test_problematic_cli_error_messages(
+    problematic_nwbfile_path_1: pathlib.Path, temporary_bids_directory: pathlib.Path
+):
     command = f"nwb2bids convert {problematic_nwbfile_path_1} -o {temporary_bids_directory}"
 
     result = subprocess.run(args=command, shell=True, capture_output=True)
     assert result.returncode == 0
-    assert b"4 suggestions for improvement were found during conversion." in result.stdout
+
+    expected_message = [
+        "",
+        "BIDS dataset was not successfully created!",
+        "Some errors were encountered during conversion.",
+        "The first 3 of 4 are shown below:",
+        "",
+        "",
+        "\t- Participant species is not a proper Latin binomial or NCBI Taxonomy id.",
+        "\t- The participant ID contains invalid characters. BIDS allows only the "
+        "plus sign to be used as a separator in the subject entity label. "
+        "Underscores, dashes, spaces, slashes, and other special characters "
+        "(including #) are expressly forbidden.",
+        "\t- Participant sex is not one of the allowed patterns by BIDS.",
+        "",
+        "",
+    ]
+    assert expected_message == result.stdout.decode(encoding="utf-8").splitlines()
+    assert result.stderr == b""
+
+
+def test_problematic_cli_critical_messages(
+    problematic_nwbfile_path_3: pathlib.Path, temporary_bids_directory: pathlib.Path
+):
+    command = f"nwb2bids convert {problematic_nwbfile_path_3} -o {temporary_bids_directory}"
+
+    result = subprocess.run(args=command, shell=True, capture_output=True)
+    assert result.returncode == 0
+
+    expected_message = ["", "BIDS dataset was successfully created, but may not be valid!", ""]
+    assert expected_message == result.stdout.decode(encoding="utf-8").splitlines()
+    assert result.stderr == b""
+
+
+def test_problematic_cli_info_messages(
+    problematic_nwbfile_path_4: pathlib.Path, temporary_bids_directory: pathlib.Path
+):
+    command = f"nwb2bids convert {problematic_nwbfile_path_4} -o {temporary_bids_directory}"
+
+    result = subprocess.run(args=command, shell=True, capture_output=True)
+    assert result.returncode == 0
+
+    expected_message = [
+        "",
+        "BIDS dataset was successfully created!",
+        "1 suggestion for improvement was found during conversion.",
+        "",
+        "",
+    ]
+    assert expected_message == result.stdout.decode(encoding="utf-8").splitlines()
+    assert result.stderr == b""
+
+
+def test_problematic_cli_success(minimal_nwbfile_path: pathlib.Path, temporary_bids_directory: pathlib.Path):
+    command = f"nwb2bids convert {minimal_nwbfile_path} -o {temporary_bids_directory}"
+
+    result = subprocess.run(args=command, shell=True, capture_output=True)
+    assert result.returncode == 0
+
+    expected_message = ["", "BIDS dataset was successfully created!", "", ""]
+    assert expected_message == result.stdout.decode(encoding="utf-8").splitlines()
     assert result.stderr == b""
 
 

tests/integration/test_messages.py

@@ -176,3 +176,26 @@ def test_messages_3(problematic_nwbfile_path_3: pathlib.Path, temporary_bids_dir
         )
     ]
     assert messages == expected_messages
+
+
+def test_messages_4(problematic_nwbfile_path_4: pathlib.Path, temporary_bids_directory: pathlib.Path) -> None:
+    nwb_paths = [problematic_nwbfile_path_4]
+    run_config = nwb2bids.RunConfig(bids_directory=temporary_bids_directory)
+    converter = nwb2bids.convert_nwb_dataset(nwb_paths=nwb_paths, run_config=run_config)
+    messages = converter.messages
+
+    expected_messages = [
+        nwb2bids.InspectionResult(
+            title="Missing description",
+            reason="A basic description of this field is recommended to improve contextual understanding.",
+            solution="Add a description to the field.",
+            examples=None,
+            field="nwbfile.devices.DeviceWithoutDescription",
+            source_file_paths=None,
+            target_file_paths=None,
+            data_standards=[nwb2bids.DataStandard.BIDS, nwb2bids.DataStandard.NWB],
+            category=nwb2bids.Category.STYLE_SUGGESTION,
+            severity=nwb2bids.Severity.INFO,
+        )
+    ]
+    assert messages == expected_messages

tests/integration/test_remote_convert_nwb_dataset.py

@@ -122,8 +122,8 @@ def test_remote_convert_nwb_dataset_on_gotten_datalad_file(
                 # TODO: in follow-up, fix the bug preventing electrodes and probes files from being created
                 # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_electrodes.json",
                 # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_electrodes.tsv",
-                # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.json",
-                # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.tsv",
+                "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.json",
+                "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.tsv",
                 "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_events.json",
                 "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_events.tsv",
             },
@@ -182,8 +182,8 @@ def test_remote_convert_nwb_dataset_on_partial_datalad_dataset(
                 # TODO: in follow-up, fix the bug preventing electrodes and probes files from being created
                 # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_electrodes.json",
                 # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_electrodes.tsv",
-                # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.json",
-                # "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.tsv",
+                "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.json",
+                "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.tsv",
                 "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_events.json",
                 "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_events.tsv",
             },