Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New Check]: Non-required Electrode columns (nwb-schema >= 2.5.0) #225

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from
20 changes: 18 additions & 2 deletions nwbinspector/checks/ecephys.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
"""Check functions specific to extracellular electrophysiology neurodata types."""
import numpy as np
from packaging import version

import numpy as np
from pynwb.file import ElectrodeTable
from pynwb.misc import Units
from pynwb.ecephys import ElectricalSeries

from hdmf.utils import get_data_shape

from ..register_checks import register_check, Importance, InspectorMessage
from ..utils import get_package_version


@register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=Units)
Expand Down Expand Up @@ -79,3 +81,17 @@ def check_spike_times_not_in_unobserved_interval(units_table: Units, nunits: int
"observed intervals."
)
)


@register_check(importance=Importance.BEST_PRACTICE_SUGGESTION, neurodata_type=ElectrodeTable)
def check_optional_columns_electrode_table(electrode_table: ElectrodeTable):
"""Check that the ElectrodeTable columns which are optional for nwb-schema>=2.5.0 are not specified as NaN."""
if get_package_version(name="pynwb") < version.Version("2.1.0"):
return
if any(x.isnan() in electrode_table for x in ["x", "y", "z", "imp", "filtering"]):
return InspectorMessage(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would yield here and say in the message which column is Nan. Also, does isnan work for an array? Maybe we could say if np.all(np.isnan(col))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, do you want it to report the exact index(ices) that are NaN or just the column that it was found in?

message=(
"The ElectrodeTable contains NaN values on optional columns - "
"as of nwb-schema 2.5.0, there is no need to write these columns."
)
)
45 changes: 45 additions & 0 deletions tests/unit_tests/test_ecephys.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
from datetime import datetime
from unittest import TestCase
from uuid import uuid4
from packaging import version

import pytest
import numpy as np
from pynwb import NWBFile
from pynwb.file import ElectrodeTable, ElectrodeGroup, Device
from pynwb.ecephys import ElectricalSeries
from pynwb.misc import Units
from hdmf.common.table import DynamicTableRegion, DynamicTable
Expand All @@ -15,7 +18,9 @@
check_electrical_series_dims,
check_electrical_series_reference_electrodes_table,
check_spike_times_not_in_unobserved_interval,
check_optional_columns_electrode_table,
)
from nwbinspector.utils import get_package_version


def test_check_negative_spike_times_all_positive():
Expand Down Expand Up @@ -220,3 +225,43 @@ def test_check_spike_times_not_in_unobserved_interval_multiple_units():
object_name="TestUnits",
location="/",
)


@pytest.mark.skipif(get_package_version(name="pynwb") < version.Version("2.1.0"))
def test_check_optional_columns_electrode_table_pass():
electrode_table = ElectrodeTable(name="electrodes")
electrode_table.add_row(
location="unknown",
group=ElectrodeGroup(name="test_group", description="", device=Device(name="test_device"), location="unknown"),
group_name="test_group",
)
assert check_optional_columns_electrode_table(electrode_table=electrode_table) is None


@pytest.mark.skip # TODO: when fixed on PyNWB side, unskip
def test_check_optional_columns_electrode_table_fail():
electrode_table = ElectrodeTable(name="electrodes")
electrode_table.add_row(
x=np.nan,
y=np.nan,
z=np.nan,
imp=np.nan,
location="unknown",
filtering="unknown",
group=ElectrodeGroup(name="test_group", description="", device=Device(name="test_device"), location="unknown"),
group_name="test_group",
)
if get_package_version(name="pynwb") >= version.Version("2.1.0"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic should go inside the skip statement. From the comments it seems like maybe you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is temporary until they fix the bug - this PR will have to stay in draft until then.

assert check_optional_columns_electrode_table(electrode_table=electrode_table) == InspectorMessage(
message=(
"The ElectrodeTable contains NaN values on optional columns - "
"as of nwb-schema 2.5.0, there is no need to write these columns."
),
importance=Importance.BEST_PRACTICE_SUGGESTION,
check_function_name="check_optional_columns_electrode_table",
object_type="ElectrodeTable",
object_name="electrodes",
location="/",
)
else:
assert check_optional_columns_electrode_table(electrode_table=electrode_table) is None