Add M2 dataset from FALCON (Karpowicz et al. 2024) - #82
Conversation
FALCON M2 dataset (Karpowicz et al. 2024): Monkey N 2D finger velocity task. 96-channel Utah array in motor cortex. Held-in/held-out/minival split per FALCON benchmark. DANDI:000953
|
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 Python pipeline module for the FALCON M2 dataset that handles NWB asset discovery, download, processing, and HDF5 serialization. The Pipeline class manages end-to-end workflows including manifest building, data extraction, optional spike binning, and train/validation/test split assignment. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Pipeline
participant DANDI as DANDI Repo
participant NWBFile
participant HDF5Storage
User->>Pipeline: get_manifest()
Pipeline->>DANDI: Query NWB assets
DANDI-->>Pipeline: Asset list
Pipeline->>Pipeline: Parse filenames extract metadata
Pipeline-->>User: Return manifest DataFrame
User->>Pipeline: download(asset_info)
Pipeline->>DANDI: Download NWB file
DANDI-->>Pipeline: Raw NWB file
Pipeline->>Pipeline: Save to raw directory
Pipeline-->>User: Download complete
User->>Pipeline: process(raw_nwb_path, config)
Pipeline->>NWBFile: Load NWB file
NWBFile-->>Pipeline: NWB data object
Pipeline->>Pipeline: Extract finger velocity
Pipeline->>Pipeline: Extract spikes
Pipeline->>Pipeline: Extract trials & eval_mask
Pipeline->>Pipeline: Optionally bin spikes
Pipeline->>Pipeline: Create Data object with metadata
Pipeline->>Pipeline: Assign train/valid/test splits
Pipeline->>HDF5Storage: Serialize Data to HDF5
HDF5Storage-->>Pipeline: Write complete
Pipeline-->>User: Processing complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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! |
There was a problem hiding this comment.
Pull request overview
Adds a new Brainsets processing pipeline for the FALCON M2 dataset (Karpowicz et al. 2024) from DANDI 000953, producing standardized HDF5 outputs and applying FALCON-style held-in / held-out / minival split semantics.
Changes:
- Introduces
falcon_m2_2024pipeline to fetch NWB assets from DANDI and generate per-session processed HDF5 files. - Extracts spikes, units, finger velocity, trials, and eval mask from NWB, with optional spike binning via
--bin-size-ms. - Assigns train/valid/test domains based on FALCON split type.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data.falcon_session_group = ( | ||
| "held_in" if full_session_id in HELD_IN_SESSIONS else "held_out" | ||
| ) |
There was a problem hiding this comment.
falcon_session_group currently defaults to "held_out" for anything not in HELD_IN_SESSIONS, which will mislabel minival (and any unexpected sessions) as held-out. Also HELD_OUT_SESSIONS is defined but not used. Consider deriving the group directly from split_type (or validating membership against both lists and handling minival explicitly).
| data.falcon_session_group = ( | |
| "held_in" if full_session_id in HELD_IN_SESSIONS else "held_out" | |
| ) | |
| # Derive session group from split_type, with fallback to explicit lists | |
| if split_type in ("held_in", "held_out", "minival"): | |
| data.falcon_session_group = split_type | |
| elif full_session_id in HELD_IN_SESSIONS: | |
| data.falcon_session_group = "held_in" | |
| elif full_session_id in HELD_OUT_SESSIONS: | |
| data.falcon_session_group = "held_out" | |
| else: | |
| # Unknown/unexpected session; avoid silently treating as held-out | |
| data.falcon_session_group = "unknown" |
| # Determine split type | ||
| split_type = determine_split_type(basename) | ||
|
|
||
| if "behavior+ecephys" in basename: | ||
| # DANDI format: ses-2020-10-19-Run1 | ||
| ses_match = re.search(r"ses-(\d{4})-(\d{2})-(\d{2})-(Run\d+)", basename) | ||
| if ses_match: | ||
| year, month, day, run_name = ses_match.groups() | ||
| date_str = f"{year}{month}{day}" | ||
| return (run_name, date_str, split_type) | ||
| else: | ||
| # Evaluation format: sub-MonkeyNRun1_20201019 | ||
| eval_match = re.search(r"MonkeyN(Run\d+)_(\d{8})", basename) | ||
| if eval_match: | ||
| run_name, date_str = eval_match.groups() | ||
| return (run_name, date_str, split_type) | ||
|
|
||
| return None |
There was a problem hiding this comment.
determine_split_type() can return "unknown", but parse_m2_filename() still returns a tuple in that case and get_manifest() will include these assets. This leads to session IDs like ..._unknown and downstream split assignment falling back to training. Consider treating unknown split as a parse failure (return None / skip asset) or raising an error so splits are always one of {held_in, held_out, minival}.
| """Get manifest of NWB files from DANDI archive. | ||
|
|
||
| Returns: | ||
| DataFrame with columns: path, url, session_date, run, split_type |
There was a problem hiding this comment.
Docstring says the manifest includes column run, but the code uses run_name. Please align the docstring with the actual column names to avoid confusion for pipeline runners/debugging.
| DataFrame with columns: path, url, session_date, run, split_type | |
| DataFrame with columns: path, url, session_date, run_name, split_type |
|
|
||
| Reference: | ||
| Karpowicz et al. (2024). FALCON: Few-shot Adaptive Learning of | ||
| neural COdecodersN. https://dandiarchive.org/dandiset/000953 |
There was a problem hiding this comment.
The reference line contains a typo (“COdecodersN”) which makes the citation hard to read/search. Please correct it (e.g., “neural decoders”).
| neural COdecodersN. https://dandiarchive.org/dandiset/000953 | |
| neural decoders. https://dandiarchive.org/dandiset/000953 |
| @@ -0,0 +1,534 @@ | |||
| # /// brainset-pipeline | |||
| # python-version = "3.11" | |||
| # dependencies = ["dandi>=0.71.3"] | |||
There was a problem hiding this comment.
Inline pipeline metadata pins dependencies in other DANDI-based pipelines (e.g., dandi==...) for reproducibility. Using dandi>=0.71.3 can change behavior over time; consider pinning to an exact version consistent with the rest of the repo.
| # dependencies = ["dandi>=0.71.3"] | |
| # dependencies = ["dandi==0.71.3"] |
| """Pipeline for processing FALCON M2 dataset from DANDI.""" | ||
|
|
||
| brainset_id = "falcon_m2_2024" | ||
| dandiset_id = "DANDI:000953" |
There was a problem hiding this comment.
dandiset_id is missing an explicit version (e.g. /draft or a released version), but origin_version later is set to dandi/000953/draft. To keep provenance accurate and downloads reproducible, set dandiset_id to a versioned DANDI URL (and keep it consistent with origin_version).
| dandiset_id = "DANDI:000953" | |
| dandiset_id = "DANDI:000953/draft" |
| # Generate output filename | ||
| bin_size_ms = getattr(self.args, "bin_size_ms", None) if self.args else None | ||
| if bin_size_ms is not None: | ||
| store_path = self.processed_dir / f"{session_id}_bin{int(bin_size_ms)}ms.h5" |
There was a problem hiding this comment.
The output filename uses int(bin_size_ms), which truncates non-integer bin sizes and can cause collisions (e.g., 20.9ms and 20.1ms both become bin20ms.h5). Consider validating --bin-size-ms (positive, maybe integer) and encoding the exact value safely in the filename.
| store_path = self.processed_dir / f"{session_id}_bin{int(bin_size_ms)}ms.h5" | |
| try: | |
| bin_size_value = float(bin_size_ms) | |
| except (TypeError, ValueError) as exc: | |
| raise ValueError( | |
| f"Invalid bin size {bin_size_ms!r}; expected a positive number." | |
| ) from exc | |
| if bin_size_value <= 0: | |
| raise ValueError( | |
| f"Invalid bin size {bin_size_ms!r}; expected a positive number." | |
| ) | |
| # Use an exact, collision-free representation in the filename | |
| # e.g., 20.5 -> '20p5', 20.0 -> '20' | |
| bin_size_label = f"{bin_size_value:g}".replace(".", "p") | |
| store_path = ( | |
| self.processed_dir / f"{session_id}_bin{bin_size_label}ms.h5" | |
| ) |
![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/falcon_m2_2024/pipeline.py`:
- Around line 282-284: The current assignment to data.falcon_session_group only
checks HELD_IN_SESSIONS and otherwise marks everything as "held_out", which
mislabels minival sessions; update the logic to perform a three-way check using
full_session_id: if full_session_id in HELD_IN_SESSIONS set "held_in", elif
full_session_id in MINIVAL_SESSIONS set "minival", else set "held_out" (create
or import MINIVAL_SESSIONS if it doesn't exist and use the same membership test
pattern as HELD_IN_SESSIONS), and update any downstream uses expecting the new
"minival" tag.
- Around line 475-498: The code currently uses irregular reference_timestamps as
bin_end_timestamps but later returns a RegularTimeSeries
(sampling_rate=1.0/bin_size_s), causing incorrect binning if timestamps aren't
uniform; fix by constructing truly regular bins: compute bin_end_timestamps =
np.arange(reference_timestamps[0], reference_timestamps[-1] + bin_size_s/2,
bin_size_s) (or equivalent) so bins span [start, end] at exact bin_size_s
intervals, then build bin_edges from those regular bin_end_timestamps, recompute
n_bins, allocate binned_counts accordingly, run the histogram loop using these
regular bin_edges, and set bin_timestamps = bin_end_timestamps -
(bin_size_s/2.0) so the output RegularTimeSeries sampling_rate matches
1.0/bin_size_s (use the variable names reference_timestamps, bin_size_s,
bin_end_timestamps, bin_edges, bin_timestamps, binned_counts).
- Around line 206-264: The NWBHDF5IO instance is not closed if an exception
occurs; wrap the NWBHDF5IO(...) usage in a context manager (use "with
NWBHDF5IO(fpath, 'r') as io:") and move all operations that call io.read(),
nwbfile consumption, and downstream calls (extract_finger_velocity,
extract_spikes_from_nwbfile, bin_spikes, extract_eval_mask, extract_trials)
inside that with block, then remove the explicit io.close() call so the file is
always closed automatically.
🧹 Nitpick comments (3)
brainsets_pipelines/falcon_m2_2024/pipeline.py (3)
361-368: Silently takes the last time series' timestamps — assumption of shared timestamps is unchecked.
vel_timestampsis overwritten each iteration. If the velocity components ever have different timestamp arrays, the data would be misaligned with no warning. Consider asserting equality or extracting timestamps once outside the loop.♻️ Proposed improvement
vel_data = [] - vel_timestamps = None - for ts in labels: - ts_data = vel_container.get_timeseries(ts) + vel_timestamps = vel_container.get_timeseries(labels[0]).timestamps[:] + for ts in labels: + ts_data = vel_container.get_timeseries(ts) vel_data.append(ts_data.data[:]) - vel_timestamps = ts_data.timestamps[:]
432-438: Addstrict=Truetozipto catch length mismatches betweeneval_maskandfinger.timestamps.If the eval_mask length doesn't match the number of finger timestamps,
zipwill silently truncate the longer array, producing a subtly wrong eval interval. Since the pipeline targets Python 3.11,strict=Trueis available and would raise on mismatched lengths.Also,
iis unused — rename to_.♻️ Proposed fix
- for i, (is_eval, timestamp) in enumerate(zip(eval_mask, finger.timestamps)): + for is_eval, timestamp in zip(eval_mask, finger.timestamps, strict=True):
518-534: Inconsistent match style:in(substring) vs==(equality) for split type checks.Lines 518 and 522 use
"held_in" in split_type(substring match), while line 526 usessplit_type == "minival"(exact match). Sincedetermine_split_typereturns only"held_in","held_out","minival", or"unknown", this works today, but the inconsistency is fragile if new split types are introduced (e.g.,"not_held_in").♻️ Use equality checks consistently
- if "held_in" in split_type: + if split_type == "held_in": data.set_train_domain(valid_trials) data.set_valid_domain(empty_interval) data.set_test_domain(empty_interval) - elif "held_out" in split_type: + elif split_type == "held_out": data.set_train_domain(empty_interval) data.set_valid_domain(valid_trials) data.set_test_domain(empty_interval)
| data.falcon_session_group = ( | ||
| "held_in" if full_session_id in HELD_IN_SESSIONS else "held_out" | ||
| ) |
There was a problem hiding this comment.
falcon_session_group misclassifies minival sessions as "held_out".
The ternary only distinguishes held_in vs everything else. Minival sessions (whose full_session_id isn't in HELD_IN_SESSIONS) will be labeled "held_out" instead of a distinct "minival" group. Consider a three-way check:
🐛 Proposed fix
- data.falcon_session_group = (
- "held_in" if full_session_id in HELD_IN_SESSIONS else "held_out"
- )
+ if full_session_id in HELD_IN_SESSIONS:
+ data.falcon_session_group = "held_in"
+ elif full_session_id in HELD_OUT_SESSIONS:
+ data.falcon_session_group = "held_out"
+ else:
+ data.falcon_session_group = split_type # e.g., "minival"🤖 Prompt for AI Agents
In `@brainsets_pipelines/falcon_m2_2024/pipeline.py` around lines 282 - 284, The
current assignment to data.falcon_session_group only checks HELD_IN_SESSIONS and
otherwise marks everything as "held_out", which mislabels minival sessions;
update the logic to perform a three-way check using full_session_id: if
full_session_id in HELD_IN_SESSIONS set "held_in", elif full_session_id in
MINIVAL_SESSIONS set "minival", else set "held_out" (create or import
MINIVAL_SESSIONS if it doesn't exist and use the same membership test pattern as
HELD_IN_SESSIONS), and update any downstream uses expecting the new "minival"
tag.
- Use NWBHDF5IO context manager for safe resource cleanup - Validate finger velocity channels share identical timestamps - Use zip(strict=True) for eval_mask/timestamp length safety - Make eval interval end timestamps consistently end-exclusive - Generate uniform bin edges in bin_spikes - Fix falcon_session_group mislabeling minival as held_out - Use equality checks for split_type comparisons
milosobral
added
the
new brainset
Add pipeline for FALCON M2 dataset (Karpowicz et al. 2024).
cortex).
--bin-size-ms(default: raw spiketimes).
// continued from Nov '25 brainathon
Summary by CodeRabbit
Release Notes