Add LINK dataset (Temmar et al. 2025) - #80
Conversation
LINK dataset (Temmar et al. 2025): 312 sessions over 3.5 years from Monkey N performing self-paced finger movements. 96-channel Utah arrays (M1) with threshold crossings and 2-finger kinematics. DANDI:001201
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis change introduces a new pipeline module for processing Temmar LINK 2025 NWB data from DANDI. The Changes
Sequence DiagramsequenceDiagram
participant Pipeline
participant DANDI
participant NWBFile
participant HDF5Store
Pipeline->>DANDI: get_manifest() - List NWB assets
DANDI-->>Pipeline: Return asset list with filenames
Pipeline->>DANDI: download(manifest_item) - Fetch NWB file
DANDI-->>Pipeline: Return downloaded NWB file path
Pipeline->>NWBFile: process(fpath) - Open & parse NWB
NWBFile-->>Pipeline: Extract threshold crossings, electrodes, velocity
Pipeline->>Pipeline: Filter trials & assign split domains
Pipeline->>HDF5Store: Write per-session Brainset dataset
HDF5Store-->>Pipeline: Confirm write complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@brainsets_pipelines/temmar_link_2025/pipeline.py`:
- Around line 406-435: The function assign_within_session_split builds
train/valid masks over all trials but does not filter by trials.is_valid like
assign_temporal_split does, which risks including invalid trials if validity
logic changes; fix by first narrowing trials to only valid trials (e.g.,
valid_trials_all = trials.select_by_mask(trials.is_valid)) then compute
n_trials, train_cutoff, valid_end and create masks against that filtered set,
finally call data.set_train_domain, data.set_valid_domain using selections from
that filtered trials and keep data.set_test_domain(empty_interval) as before;
reference functions/attributes: assign_within_session_split,
assign_temporal_split, create_filtered_trials, trials.is_valid,
trials.select_by_mask, Data.set_train_domain, Data.set_valid_domain,
Data.set_test_domain.
- Around line 266-289: The two branches produce different attribute names for
spike counts — RegularTimeSeries is constructed with counts=tcfr_data while
IrregularTimeSeries uses data=tcfr_data — causing consumers to need type checks;
change the IrregularTimeSeries construction to use counts=tcfr_data (matching
RegularTimeSeries) so both branches expose the same attribute, and return the
tcfr variable as before; update any constructor argument name in the
IrregularTimeSeries call (timestamps=tcfr_timestamps, counts=tcfr_data,
domain="auto") to fix consistency.
- Around line 164-180: The NWBHDF5IO file handle is not closed if an exception
occurs during extraction; wrap the NWBHDF5IO usage in a context manager (use
"with NWBHDF5IO(fpath, 'r') as io:"), call io.read() inside the with block to
get nwbfile, then perform the calls to update_status,
extract_threshold_crossings, extract_units_metadata, extract_finger_velocity and
nwbfile.trials.to_dataframe() inside that block so the file is guaranteed to be
closed even on error; update references to io.read(), io.close(),
extract_threshold_crossings, extract_units_metadata, extract_finger_velocity,
and nwbfile.trials.to_dataframe() accordingly.
🧹 Nitpick comments (3)
brainsets_pipelines/temmar_link_2025/pipeline.py (3)
217-228: Full (unfiltered) neural and behavioral data is shared across CO and RD session files.
tcfr,finger, andunitsare extracted once from the full NWB recording and stored identically in both the CO and RD session HDF5 files. Onlytrialsand the train/valid domains differ. This effectively doubles the on-disk storage per NWB file. If intentional (simplicity over storage), a brief comment would help future maintainers. Otherwise, consider slicingtcfrandfingerto cover only the time ranges relevant to each target style's trials.
301-310: No validation thatindex_velocityandmrs_velocityshare the same timestamps.
mrs_velocitytimestamps are silently discarded, and onlyindex_velocitytimestamps are used. If the two time series ever have different time bases (same length but different values), the stacked velocity data would be silently misaligned. A quick assertion would safeguard data integrity.Proposed guard
index_vel_ts = nwbfile.analysis["index_velocity"] mrs_vel_ts = nwbfile.analysis["mrs_velocity"] timestamps = index_vel_ts.timestamps[:] + mrs_timestamps = mrs_vel_ts.timestamps[:] + assert np.array_equal(timestamps, mrs_timestamps), ( + "index_velocity and mrs_velocity timestamps do not match" + ) vel_data = np.column_stack(
205-209: Consider usingTask.FREE_BEHAVIORor creating a more specific task enum for self-paced movements.LINK contains self-paced 2-finger flexion/extension movements where target styles (CO and RD) organize trials but do not constrain the self-paced behavior.
Task.REACHINGis semantically inappropriate—it implies goal-directed reaching movements.Task.FREE_BEHAVIORis a better fit, though it may be loose given the target structure. If this pattern recurs, a dedicated enum value likeFINGER_MOVEMENTwould improve semantic clarity in the taxonomy.
There was a problem hiding this comment.
Pull request overview
This pull request adds a new pipeline (temmar_link_2025) for processing the LINK dataset (Temmar et al. 2025) from DANDI archive 001201. The dataset contains 312 recording sessions spanning 3.5 years from Monkey N performing self-paced finger movements, with 96-channel Utah array threshold crossings and 2-finger kinematics data.
Changes:
- New pipeline implementation for the LINK dataset with support for two split strategies (temporal and within-session)
- Extraction of threshold crossing neural data and finger velocity behavioral data from NWB files
- Processing of different target styles (CO and RD) as separate sessions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tcfr_ts = nwbfile.analysis["ThresholdCrossings"] | ||
| tcfr_data = tcfr_ts.data[:].astype(np.int16) # (time, 96) | ||
| tcfr_timestamps = tcfr_ts.timestamps[:] | ||
|
|
There was a problem hiding this comment.
The code assumes tcfr_timestamps has at least 2 elements when calling np.diff at line 271. If the array has fewer than 2 elements, np.diff will return an empty array, causing the indexing at line 272 (dt[0]) to fail with an IndexError. Add a check to handle this edge case, for example by checking if len(tcfr_timestamps) < 2 before attempting the regularity check.
| # Handle short recordings that don't allow a regularity check | |
| if tcfr_timestamps.size < 2: | |
| tcfr = IrregularTimeSeries( | |
| timestamps=tcfr_timestamps, | |
| data=tcfr_data, | |
| domain="auto", | |
| ) | |
| return tcfr |
| else: | ||
| tcfr = IrregularTimeSeries( | ||
| timestamps=tcfr_timestamps, | ||
| data=tcfr_data, |
There was a problem hiding this comment.
The IrregularTimeSeries is created with data=tcfr_data, but the RegularTimeSeries uses counts=tcfr_data. For consistency and to match the naming convention used elsewhere in the codebase for threshold crossing data, both should use the same attribute name (counts). This naming inconsistency could lead to confusion when accessing the data later.
| data=tcfr_data, | |
| counts=tcfr_data, |
| def extract_units_metadata(nwbfile): | ||
| """Extract electrode/channel metadata as units. | ||
|
|
||
| LINK has 96 channels (2 Utah arrays, 8x8 each). |
There was a problem hiding this comment.
The comment states "2 Utah arrays, 8x8 each" which would total 128 channels (2 * 8 * 8 = 128), but the module docstring and description consistently mention 96 channels. This is a mathematical inconsistency. Please verify the actual array configuration and update the comment accordingly.
| LINK has 96 channels (2 Utah arrays, 8x8 each). | |
| LINK has 96 channels recorded from Utah arrays. |
| """ | ||
| electrodes_df = nwbfile.electrodes.to_dataframe() | ||
|
|
||
| unit_ids = np.arange(len(electrodes_df), dtype=np.int32) |
There was a problem hiding this comment.
Unit IDs are set to integer values (unit_ids = np.arange(len(electrodes_df), dtype=np.int32)), but other pipelines in the codebase use string-based unit IDs (e.g., "group_{group_name}/elec{i}/multiunit_{0}" in dandi_utils.py). Consider using descriptive string IDs that incorporate array_name, bank, row, and col information for better identification and consistency with the rest of the codebase.
| unit_ids = np.arange(len(electrodes_df), dtype=np.int32) | |
| # Use descriptive string-based unit IDs incorporating electrode metadata | |
| unit_ids = ( | |
| electrodes_df["array_name"].astype(str) | |
| + "_bank" + electrodes_df["bank"].astype(str) | |
| + "_row" + electrodes_df["row"].astype(str) | |
| + "_col" + electrodes_df["col"].astype(str) | |
| ).values |
| first_val = filtered_df[col].iloc[0] if len(filtered_df) > 0 else None | ||
| if isinstance(first_val, str): |
There was a problem hiding this comment.
The logic to drop string columns only checks the first value in each column (line 368). If the first value is None or NaN but later values in the column are strings, the column won't be dropped, which could still cause HDF5 serialization issues. Consider checking all values or using a more robust method like checking if any non-null value in the column is a string.
| first_val = filtered_df[col].iloc[0] if len(filtered_df) > 0 else None | |
| if isinstance(first_val, str): | |
| non_null_values = filtered_df[col].dropna() | |
| if not non_null_values.empty and any( | |
| isinstance(v, str) for v in non_null_values | |
| ): |
| @@ -0,0 +1,435 @@ | |||
| # /// brainset-pipeline | |||
| # python-version = "3.11" | |||
| # dependencies = ["dandi>=0.71.3"] | |||
There was a problem hiding this comment.
The dependency is specified as "dandi>=0.71.3" using a minimum version constraint, but other pipelines in the codebase consistently use exact version pinning (e.g., "dandi==0.71.3" in pei_pandarinath_nlb_2021). For consistency and reproducibility, consider changing this to an exact version pin like "dandi==0.71.3".
| # dependencies = ["dandi>=0.71.3"] | |
| # dependencies = ["dandi==0.71.3"] |
| subject=subject, | ||
| session=session_description, | ||
| device=device_description, | ||
| tcfr=tcfr, |
There was a problem hiding this comment.
The neural data is named 'tcfr' in the Data object, but other pipelines in the codebase consistently use 'spikes' as the attribute name for neural activity, even when the data represents threshold crossings (see churchland_shenoy_neural_2012/pipeline.py:152, 601). For consistency with the rest of the codebase, consider renaming 'tcfr' to 'spikes'.
| # Extract neural activity (pre-binned threshold crossings) | ||
| self.update_status("Extracting Neural Data") | ||
| tcfr = extract_threshold_crossings(nwbfile) | ||
| units = extract_units_metadata(nwbfile) | ||
|
|
||
| # Extract finger velocity | ||
| self.update_status("Extracting Behavior") | ||
| finger = extract_finger_velocity(nwbfile) |
There was a problem hiding this comment.
Neural and behavioral data (tcfr and finger) are extracted once and then reused for all target styles. If different target styles should have different data (filtered by trial times), these extractions should either be moved inside the target_style loop or the data should be filtered based on the trial intervals for each target style. Currently, all sessions (CO and RD) will contain the same complete neural and behavioral data, which may not match the filtered trials.
milosobral
added
the
new brainset
Add temmar_link_2025 pipeline for the LINK dataset (Temmar et al. 2025).
// continued from Nov '25 brainathon
Summary by CodeRabbit