Add M1-A dataset from FALCON (Karpowicz et al. 2024) - #81
Conversation
FALCON M1 dataset (Karpowicz et al. 2024): Monkey L reach-to-grasp task. 64-channel Utah array in motor cortex with 16-channel EMG recordings. Held-in/held-out/minival split per FALCON benchmark. DANDI:000941
|
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. 📝 WalkthroughWalkthroughIntroduces a new FALCON M1 2024 data pipeline that downloads and processes DANDI NWB files, extracting neural spike and EMG data with optional spike binning, metadata enrichment, and HDF5 serialization according to dataset-specific split rules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@brainsets_pipelines/falcon_m1_2024/pipeline.py`:
- Around line 384-390: The end timestamps pushed into eval_mask_ends are
inconsistent: mid-session you append the first non-eval sample timestamp
(`eval_mask_ends.append(timestamp)`), but at session end you append
`emg.timestamps[-1]`; decide whether Interval expects end-exclusive
(one-past-last) or end-inclusive (last-sample) and make both branches match that
convention. If Interval is end-exclusive, change the session-end branch to
append a one-past-last timestamp (e.g., compute sample interval from
`emg.timestamps` and append `emg.timestamps[-1] + dt`); if Interval is
end-inclusive, change the mid-session branch to append the last-eval sample
timestamp (e.g., `previous_timestamp` or `emg.timestamps[idx-1]`). Update the
code paths that build `eval_mask_ends`, referencing the variables
`in_eval_period`, `eval_mask_ends`, `timestamp`, and `emg.timestamps` (and
`Interval` semantics) so both transitions use the same end-of-interval
convention.
- Around line 373-399: The eval_mask/EMG timestamps loop can silently truncate
when lengths differ; update the logic that builds eval_intervals (the loop over
eval_mask and emg.timestamps) to first validate len(eval_mask) ==
len(emg.timestamps) and raise a clear error if not, or use zip(..., strict=True)
to force a mismatch exception (requires Python 3.10+); also remove the unused
enumerate and i variable so the loop simply iterates over paired values. Ensure
the error message references eval_mask and emg.timestamps so callers can
diagnose the length mismatch before constructing the Interval objects.
- Around line 189-247: The NWBHDF5IO file handle is not protected against
exceptions causing a resource leak; wrap the IO usage in a context manager by
replacing the manual open/close with a with NWBHDF5IO(fpath, "r") as io: block,
move the nwbfile = io.read() and all subsequent code that accesses nwbfile
(calls to extract_emg_data, extract_spikes_from_nwbfile, bin_spikes,
extract_eval_mask, extract_trials, and any use of spikes/units) inside that with
block, and remove the explicit io.close() call so the file is always closed even
on errors.
- Around line 421-448: The current bin_edges are built from reference_timestamps
which yields non-uniform widths; instead generate uniform edges using bin_size_s
so each bin has width bin_size_s. Replace the construction of bin_edges
(currently using reference_timestamps / bin_end_timestamps) with a uniform
sequence starting at (reference_timestamps[0] - bin_size_s) and stepping by
bin_size_s for n_bins+1 edges (so bin_edges = start + np.arange(n_bins+1) *
bin_size_s). Keep the histogram loop over units (spike_times / spike_units) but
use the new uniform bin_edges, and compute bin_timestamps as the midpoints of
those uniform edges so the returned binned_counts and bin_timestamps align with
RegularTimeSeries(sampling_rate=1/bin_size_s).
🧹 Nitpick comments (1)
brainsets_pipelines/falcon_m1_2024/pipeline.py (1)
309-316:emg_timestampsis silently overwritten each iteration — assumes all muscles share identical timestamps.If any muscle channel has different timestamps, the mismatch would go undetected and the resulting
IrregularTimeSerieswould pair incorrect timestamps with stacked data. Consider adding a validation or extracting timestamps only once:Suggested defensive check
emg_data = [] emg_timestamps = None for muscle in muscles: ts_data = emg_container.get_timeseries(muscle) emg_data.append(ts_data.data[:]) - emg_timestamps = ts_data.timestamps[:] + if emg_timestamps is None: + emg_timestamps = ts_data.timestamps[:] + else: + assert np.array_equal(emg_timestamps, ts_data.timestamps[:]), ( + f"Timestamp mismatch for muscle {muscle}" + )
There was a problem hiding this comment.
Pull request overview
This PR adds a new pipeline for processing the FALCON M1 dataset (Karpowicz et al. 2024) from DANDI archive 000941. The dataset contains neural recordings from a 64-channel Utah array in motor cortex of a macaque monkey performing a reach-to-grasp task, along with 16-channel EMG recordings.
Changes:
- New pipeline implementation for FALCON M1 dataset with support for held-in/held-out/minival split convention
- Optional spike binning feature via
--bin-size-msargument (defaults to raw spike times) - EMG extraction and evaluation mask processing for FALCON benchmark compatibility
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for muscle in muscles: | ||
| ts_data = emg_container.get_timeseries(muscle) | ||
| emg_data.append(ts_data.data[:]) | ||
| emg_timestamps = ts_data.timestamps[:] |
There was a problem hiding this comment.
In the extract_emg_data function, emg_timestamps is overwritten in each loop iteration. This assumes all muscle time series have identical timestamps. If they differ, only the last muscle's timestamps will be used, which could cause a mismatch with the stacked data. Consider verifying that all timestamps are identical or using the first muscle's timestamps explicitly.
| emg_timestamps = ts_data.timestamps[:] | |
| ts_timestamps = ts_data.timestamps[:] | |
| if emg_timestamps is None: | |
| emg_timestamps = ts_timestamps | |
| else: | |
| if not np.array_equal(ts_timestamps, emg_timestamps): | |
| raise ValueError( | |
| "EMG timestamps are not identical across muscles; " | |
| "cannot construct a consistent IrregularTimeSeries." | |
| ) |
| eval_mask_ends = [] | ||
| in_eval_period = False | ||
|
|
||
| for i, (is_eval, timestamp) in enumerate(zip(eval_mask, emg.timestamps)): |
There was a problem hiding this comment.
The enumerate index variable 'i' is not used. Consider using 'for is_eval, timestamp in zip(eval_mask, emg.timestamps):' instead for cleaner code.
| for i, (is_eval, timestamp) in enumerate(zip(eval_mask, emg.timestamps)): | |
| for is_eval, timestamp in zip(eval_mask, emg.timestamps): |
| # Bin center timestamps | ||
| bin_timestamps = bin_end_timestamps - (bin_size_s / 2.0) |
There was a problem hiding this comment.
The bin_spikes function assumes that reference_timestamps are evenly spaced when calculating bin centers (line 446). However, emg.timestamps is from an IrregularTimeSeries (line 228), which may have irregularly spaced timestamps. This could lead to inaccurate bin center calculations. Consider either: 1) verifying that EMG timestamps are regularly spaced, or 2) calculating bin centers directly from bin_edges as (bin_edges[:-1] + bin_edges[1:]) / 2.
| # Bin center timestamps | |
| bin_timestamps = bin_end_timestamps - (bin_size_s / 2.0) | |
| # Bin center timestamps computed from bin edges to avoid assuming regular spacing | |
| bin_timestamps = (bin_edges[:-1] + bin_edges[1:]) / 2.0 |
| subject = SubjectDescription( | ||
| id="monkey_l", | ||
| species=Species.MACACA_MULATTA, | ||
| sex=Sex.UNKNOWN, | ||
| ) |
There was a problem hiding this comment.
Subject metadata is hardcoded instead of being extracted from the NWB file. Consider using extract_subject_from_nwb (from brainsets.utils.dandi_utils) which is available and used in other DANDI-based pipelines (e.g., pei_pandarinath_nlb_2021). This would make the code more maintainable and consistent with other pipelines.
| eval_mask_ends = [] | ||
| in_eval_period = False | ||
|
|
||
| for i, (is_eval, timestamp) in enumerate(zip(eval_mask, emg.timestamps)): |
There was a problem hiding this comment.
The function zips eval_mask and emg.timestamps without verifying they have the same length. If these arrays have different lengths, the shorter one will silently truncate the iteration, potentially missing evaluation periods. Consider adding a length check or assertion to ensure data integrity.
| else: | ||
| # Unknown split, default to train | ||
| data.set_train_domain(valid_trials) | ||
| data.set_valid_domain(empty_interval) | ||
| data.set_test_domain(empty_interval) |
There was a problem hiding this comment.
When split_type is "unknown" (line 294), the assign_falcon_split function silently assigns it to the training set without any warning or logging. This could mask configuration issues or unexpected filenames. Consider adding a warning log when encountering unknown split types to help with debugging.
|
|
||
| Reference: | ||
| Karpowicz et al. (2024). FALCON: Few-shot Adaptive Learning of | ||
| neural COdecodersN. https://dandiarchive.org/dandiset/000941 |
There was a problem hiding this comment.
The reference has a typo in the acronym expansion. It should be "COdecoders" not "COdecodersN".
| neural COdecodersN. https://dandiarchive.org/dandiset/000941 | |
| neural COdecoders. https://dandiarchive.org/dandiset/000941 |
| @@ -0,0 +1,482 @@ | |||
| # /// brainset-pipeline | |||
| # python-version = "3.11" | |||
| # dependencies = ["dandi>=0.71.3"] | |||
There was a problem hiding this comment.
The dependency specification uses a range operator (>=) which is inconsistent with other pipelines in the codebase that use exact version pinning (==). For reproducibility, consider changing "dandi>=0.71.3" to "dandi==0.71.3".
| # dependencies = ["dandi>=0.71.3"] | |
| # dependencies = ["dandi==0.71.3"] |
- Use NWBHDF5IO context manager for safe resource cleanup - Validate eval_mask / EMG timestamp length match - Make eval interval end timestamps consistently end-exclusive - Generate uniform bin edges in bin_spikes - Validate EMG muscle channels share identical timestamps
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
milosobral
added
the
new brainset
Add pipeline for FALCON M1 dataset (Karpowicz et al. 2024).
16-channel EMG.
--bin-size-ms(default: raw spiketimes).
// continued from Nov '25 brainathon
Summary by CodeRabbit