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

Fix attribute retrieval from version-dependent icephys #264

Merged
merged 9 commits into from
Sep 23, 2022
Merged
32 changes: 32 additions & 0 deletions .github/workflows/version_gallery.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: Past PyNWB Version
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason you are setting this up as a cron job instead of just adding it to the PR triggered workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multiplicity - If I include it in the main workflow we'd have (number of platforms) x (number of Python versions) x (number of PyNWB versions), which gets a bit out of hand.

This instead constrains to ubuntu-latest + Python 3.9 and then goes back as far in the version history as desired.

on:
schedule:
- cron: "0 0 * * *" # daily
pull_request:

jobs:
build-and-test:
name: Testing against past PyNWB versions
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
pynwb-version: ["2.1.0", "2.0.0"]
steps:
- uses: s-weigand/setup-conda@v1
with:
update-conda: true
python-version: 3.9
conda-channels: conda-forge
- uses: actions/checkout@v2
- run: git fetch --prune --unshallow --tags
- name: Install pytest
run: |
pip install pytest
pip install pytest-cov
- name: Install package
run: |
pip install -e .
pip install pynwb==${{ matrix.pynwb-version }}
- name: Run pytest with coverage
run: pytest -rsx
2 changes: 1 addition & 1 deletion nwbinspector/checks/icephys.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@
@register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=IntracellularElectrode)
def check_intracellular_electrode_cell_id_exists(intracellular_electrode: IntracellularElectrode):
"""Check if the IntracellularElectrode contains a cell_id."""
if intracellular_electrode.cell_id is None:
if getattr(intracellular_electrode, "cell_id", "") is None: # Will only be None with PyNWB>=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 understand that this would work, but maybe a better option is to be a bit more explicit, using get_package_version(name="pynwb") < Version("2.1.0") within the check function itself.

Copy link
Contributor Author

@CodyCBakerPhD CodyCBakerPhD Sep 20, 2022

Choose a reason for hiding this comment

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

I thought about that but that is precisely the precedence that leads us down a rabbit hole in the future to having to figure out which parts of which tests depend on which version of PyNWB is being used at runtime.

So it's a lot easier on us to set the precedence of just safely checking if an attribute is accessible and do an early return if not, independent of the reason that field might not be accessible.

Also, apologies - I had thought the last commit included an adjustment I made swapping it from getattr to hasattr but it didn't get pushed, my bad.

return InspectorMessage(message="Please include a unique cell_id associated with this IntracellularElectrode.")
15 changes: 15 additions & 0 deletions tests/unit_tests/test_icephys.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
import pytest
from packaging.version import Version
from pynwb.icephys import IntracellularElectrode
from pynwb.device import Device

from nwbinspector import InspectorMessage, Importance, check_intracellular_electrode_cell_id_exists
from nwbinspector.utils import get_package_version

PYNWB_VERSION_LOWER_2_1_0 = get_package_version(name="pynwb") < Version("2.1.0")
PYNWB_VERSION_LOWER_SKIP_REASON = "This test requires PyNWB>=2.1.0"


@pytest.mark.skipif(PYNWB_VERSION_LOWER_2_1_0, reason=PYNWB_VERSION_LOWER_SKIP_REASON)
def test_pass_check_intracellular_electrode_cell_id_exists():
device = Device(name="device")
ielec = IntracellularElectrode(name="ielec", cell_id="123", device=device, description="an intracellular electrode")
assert check_intracellular_electrode_cell_id_exists(ielec) is None


@pytest.mark.skipif(PYNWB_VERSION_LOWER_2_1_0, reason=PYNWB_VERSION_LOWER_SKIP_REASON)
def test_fail_check_intracellular_electrode_cell_id_exists():
device = Device(name="device")
ielec = IntracellularElectrode(name="ielec", device=device, description="an intracellular electrode")
Expand All @@ -21,3 +29,10 @@ def test_fail_check_intracellular_electrode_cell_id_exists():
object_name="ielec",
location="/",
)


@pytest.mark.skipif(not PYNWB_VERSION_LOWER_2_1_0, reason="This test requires PyNWB<2.1.0")
def test_skip_check_for_lower_versions():
device = Device(name="device")
ielec = IntracellularElectrode(name="ielec", device=device, description="an intracellular electrode")
assert check_intracellular_electrode_cell_id_exists(ielec) is None