diff --git a/src/nwb2bids/_command_line_interface/_main.py b/src/nwb2bids/_command_line_interface/_main.py index f837ad9d..178936a0 100644 --- a/src/nwb2bids/_command_line_interface/_main.py +++ b/src/nwb2bids/_command_line_interface/_main.py @@ -1,3 +1,4 @@ +import collections import pathlib import typing @@ -5,7 +6,7 @@ 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 diff --git a/src/nwb2bids/_tools/_pluralize.py b/src/nwb2bids/_tools/_pluralize.py index 85c06271..b27c3316 100644 --- a/src/nwb2bids/_tools/_pluralize.py +++ b/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 diff --git a/src/nwb2bids/bids_models/_bids_session_metadata.py b/src/nwb2bids/bids_models/_bids_session_metadata.py index 37dc253f..7c2dde38 100644 --- a/src/nwb2bids/bids_models/_bids_session_metadata.py +++ b/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: diff --git a/src/nwb2bids/bids_models/_probes.py b/src/nwb2bids/bids_models/_probes.py index 7d79bfbc..a272d644 100644 --- a/src/nwb2bids/bids_models/_probes.py +++ b/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 = { diff --git a/tests/conftest.py b/tests/conftest.py index 2714acba..29a3d70a 100644 --- a/tests/conftest.py +++ b/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) diff --git a/tests/convert_nwb_dataset/test_cli_messages.py b/tests/convert_nwb_dataset/test_cli_messages.py index e194bebb..5fa7e44b 100644 --- a/tests/convert_nwb_dataset/test_cli_messages.py +++ b/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"" diff --git a/tests/integration/test_messages.py b/tests/integration/test_messages.py index 9f2d40ab..c4661ca7 100644 --- a/tests/integration/test_messages.py +++ 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 diff --git a/tests/integration/test_remote_convert_nwb_dataset.py b/tests/integration/test_remote_convert_nwb_dataset.py index a2d057f4..df80b7a8 100644 --- a/tests/integration/test_remote_convert_nwb_dataset.py +++ b/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", },