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

make untyped dataset of references expandable #1188

Merged
merged 6 commits into from
Oct 23, 2024
Merged

Conversation

stephprince
Copy link
Contributor

@stephprince stephprince commented Sep 5, 2024

Motivation

Fix #1181

To do:

  • add tests

How to test the behavior?

from datetime import datetime
from uuid import uuid4
from dateutil import tz

from pynwb import NWBHDF5IO, NWBFile, H5DataIO

session_start_time = datetime(2018, 4, 25, 2, 30, 3, tzinfo=tz.gettz("US/Pacific"))

# setup NWBFile
nwbfile = NWBFile(
    session_description="Mouse exploring an open field",  # required
    identifier=str(uuid4()),  # required
    session_start_time=session_start_time,  # required
)

# add column
device = nwbfile.create_device(name="array", description="an array", manufacturer="company")

# create an electrode group
electrode_group = nwbfile.create_electrode_group(
    name="shank0",
    description="electrode group for shank 0",
    device=device,
    location="brain area",
)

# add electrode to the electrode table
nwbfile.add_electrode(group=electrode_group, location="brain area",)
nwbfile.electrodes.id.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})
nwbfile.electrodes.group.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})
nwbfile.electrodes.group_name.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})
nwbfile.electrodes.location.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})

with NWBHDF5IO("test_append_electrodes_table.nwb", "w") as io:
    io.write(nwbfile)

with NWBHDF5IO("test_append_electrodes_table.nwb", mode="a") as io:
    nwbfile = io.read()
    electrode_group = nwbfile.electrode_groups["shank0"]
    
    nchannels = 4
    for ielec in range(nchannels):
        nwbfile.add_electrode(
            group=electrode_group,
            location="brain area",
        )

    io.write(nwbfile)

with NWBHDF5IO("test_append_electrodes_table.nwb", "r") as io:
    nwbfile = io.read()
    print(nwbfile.electrodes.to_dataframe())

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Does the PR clearly describe the problem and the solution?
  • Have you reviewed our Contributing Guide?
  • Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

@stephprince
Copy link
Contributor Author

@mavaylon1 These are the changes related to our issue triaging that we will discuss testing set up for next week.

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.03%. Comparing base (dedc1dd) to head (d068f8e).
Report is 31 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1188   +/-   ##
=======================================
  Coverage   89.03%   89.03%           
=======================================
  Files          45       45           
  Lines        9883     9887    +4     
  Branches     2813     2815    +2     
=======================================
+ Hits         8799     8803    +4     
  Misses        767      767           
  Partials      317      317           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stephprince stephprince marked this pull request as ready for review October 16, 2024 21:55
@stephprince stephprince requested a review from mavaylon1 October 16, 2024 21:55
@mavaylon1 mavaylon1 merged commit b7a5fe2 into dev Oct 23, 2024
29 checks passed
@mavaylon1 mavaylon1 deleted the expand-untyped-ref-dset branch October 23, 2024 22:58
@cboulay
Copy link
Contributor

cboulay commented Jan 9, 2025

Here is an expanded test that uses 2 electrode groups and 2 electrical series linked to different electrode ranges. I was surprised by the need to call io.write(nwbfile) before adding electrodes to the second group, as this wasn't required of the first group.

from datetime import datetime
from uuid import uuid4
from dateutil import tz

import numpy as np
from pynwb import NWBHDF5IO, NWBFile, H5DataIO, ecephys

session_start_time = datetime(2018, 4, 25, 2, 30, 3, tzinfo=tz.gettz("US/Pacific"))

# setup NWBFile
nwbfile = NWBFile(
    session_description="Mouse exploring an open field",  # required
    identifier=str(uuid4()),  # required
    session_start_time=session_start_time,  # required
)

# add column
nwbfile.add_electrode_column(name="label", description="electrode label")

# add device
device = nwbfile.create_device(name="array", description="an array", manufacturer="company")

# create an electrode group
electrode_group = nwbfile.create_electrode_group(
    name="shank0",
    description="electrode group for shank 0",
    device=device,
    location="brain area",
)

# add electrode to the electrode table
n0 = 2
for ch_ix in range(n0):
    nwbfile.add_electrode(
        group=electrode_group,
        location="brain area",
        label=f"ch{ch_ix}",
    )

# Make the fields expandable
nwbfile.electrodes.id.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})
nwbfile.electrodes.group.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})
nwbfile.electrodes.group_name.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})
nwbfile.electrodes.location.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})
nwbfile.electrodes.label.set_data_io(H5DataIO, {'maxshape': (None,), 'chunks': True})

el_tbl_region = nwbfile.create_electrode_table_region(region=list(range(n0)), description=f"elec region 0")
series = ecephys.ElectricalSeries(
    name="series0",
    description="series0 descr",
    data=np.random.randn(50, n0),
    electrodes=el_tbl_region,
    starting_time=0.0,
    rate=2000.0,
)
nwbfile.add_acquisition(series)

with NWBHDF5IO("test_append_electrodes_table.nwb", "w") as io:
    io.write(nwbfile)

with NWBHDF5IO("test_append_electrodes_table.nwb", mode="a") as io:
    nwbfile = io.read()

    electrode_group = nwbfile.create_electrode_group(
        name="shank1",
        description="electrode group for shank 1",
        device=nwbfile.devices["array"],
        location="brain area",
    )
    io.write(nwbfile)  # Surprised this is necessary to avoid error.

    n1 = 4
    for ielec in range(n0, n0 + n1):
        nwbfile.add_electrode(
            group=electrode_group,
            location="brain area",
            label=f"ch{ielec}",
        )
    el_tbl_region = nwbfile.create_electrode_table_region(
        region=list(range(n0, n0 + n1)),
        description=f"elec region 1"
    )
    series = ecephys.ElectricalSeries(
        name="series1",
        description="series1 descr",
        data=np.random.randn(50, n1),
        electrodes=el_tbl_region,
        starting_time=0.1,
        rate=1000.0,
    )
    nwbfile.add_acquisition(series)

    io.write(nwbfile)

with NWBHDF5IO("test_append_electrodes_table.nwb", "r") as io:
    nwbfile = io.read()
    el_df = nwbfile.electrodes.to_dataframe()
    assert el_df.label.tolist() == [f"ch{ielec}" for ielec in range(n0 + n1)]

    eldf0 = nwbfile.acquisition["series0"].electrodes.to_dataframe()
    assert eldf0.label.tolist() == [f"ch{ielec}" for ielec in range(n0)]
    
    eldf1 = nwbfile.acquisition["series1"].electrodes.to_dataframe()
    assert eldf1.label.tolist() == [f"ch{ielec}" for ielec in range(n0, n0 + n1)]

@stephprince
Copy link
Contributor Author

Thanks for sharing @cboulay! I think this would be a great example for our pynwb documentation: NeurodataWithoutBorders/pynwb#1951.

If I remember correctly, io.write() needs to be called again because of issues creating an HDF5 reference to the electrode group object that isn't in the file yet, but @mavaylon1 could you elaborate on this and explain why it is needed when appending in particular?

@cboulay
Copy link
Contributor

cboulay commented Jan 10, 2025

By the way, thank you for adding this feature! It enabled us to get past one of the sticking points we had when streaming to pynwb.
Despite the pasted code working well, in my weird asynch ezmsg class I actually still have the error when adding new electrodes to a secondary electrode group despite calling that extra io.write so for now I'm limited to a single unified "group" per session. That's fine for us for now, but I would be interested in understanding how this works a little better to maybe identify my specific problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: untyped dataset of references cannot be set as expandable
3 participants