Add peterson_brunton_pose_trajectory_2022 (AJILE12) dataset pipeline - #87
Add peterson_brunton_pose_trajectory_2022 (AJILE12) dataset pipeline#87milosobral wants to merge 44 commits into
Conversation
- Simplified hemisphere identification functions in dandi_utils.py. - Enhanced error handling for trial folds generation in split.py to ensure sufficient trials for each category. - Updated ECoG extraction to use native sampling rate and adjusted resampling logic in pipeline.py.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the PetersonBruntonPoseTrajectory2022 brainset: new dataset class, full NWB→HDF5 pipeline, ECoG/pose extraction and hemisphere resolution utilities, task-aware stratified fold generation, tests, docs/registry updates, and related tooling/dependency adjustments. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Pipeline as Pipeline
participant DANDI as DANDI
participant NWB as NWB File
participant Processing as Signal & Pose Processing
participant Splits as Split Generation
participant HDF5 as HDF5 Output
User->>Pipeline: run prepare peterson_brunton_pose_trajectory_2022
Pipeline->>DANDI: list & download NWB assets
DANDI-->>Pipeline: NWB files
loop per session
Pipeline->>NWB: open & parse metadata
NWB-->>Pipeline: subject/session info
Pipeline->>Processing: extract ECoG (extract_ecog_from_nwb)
Processing-->>Pipeline: ECoG timeseries + channel metadata
Pipeline->>Processing: extract pose keypoints
Processing-->>Pipeline: pose timeseries
Processing->>Processing: compute pose-valid & ecog-valid domains
Processing-->>Pipeline: joint valid domain
Pipeline->>Processing: extract/trim behavior intervals
Processing-->>Pipeline: trimmed behavior intervals
Pipeline->>Splits: generate folds (inter/intra/stratified/pose)
Splits-->>Pipeline: train/valid/test intervals per fold
Pipeline->>HDF5: assemble descriptions + write session.h5
HDF5-->>Pipeline: write confirmation
end
Pipeline-->>User: processing complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
estefanysuarez
left a comment
estefanysuarez
left a comment
There was a problem hiding this comment.
Hey Milo, thanks for the PR. I left a few comments!
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
milosobral
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
brainsets/utils/split.py (1)
1-6: 🛠️ Refactor suggestion | 🟠 MajorExport the new public helper in
__all__.
generate_stratified_folds_by_taskis defined as a public function and consumed by the new pipeline (and tests import it directly), but it was not added to_functions/__all__. Adding it keeps the module's public surface discoverable and consistent with the other two helpers.♻️ Proposed fix
_functions = [ "generate_stratified_folds", + "generate_stratified_folds_by_task", "generate_string_kfold_assignment", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/split.py` around lines 1 - 6, The module's public export list misses the new helper: add "generate_stratified_folds_by_task" to the _functions list (and thus __all__) alongside "generate_stratified_folds" and "generate_string_kfold_assignment" so the function is exported; update the _functions array in the top-level of brainsets/utils/split.py to include the exact symbol name generate_stratified_folds_by_task.brainsets/datasets/__init__.py (1)
14-38:⚠️ Potential issue | 🟡 Minor
PetersonBruntonPoseTrajectory2022is imported but not added to__all__.
__all__is assembled from the_electrophysiology_datasets/_calcium_imaging_datasets/_ieeg_datasets/_psg_datasetslists, and this new class isn't added to any of them. Result:from brainsets.datasets import *does not export it, and any tooling that relies on__all__(star imports, docs autogen) will skip it. AJILE12 is an ECoG dataset, so grouping it withNeuroprobe2025under_ieeg_datasetsseems the closest fit.🐛 Proposed fix
_ieeg_datasets = [ "Neuroprobe2025", + "PetersonBruntonPoseTrajectory2022", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/datasets/__init__.py` around lines 14 - 38, The export list is missing PetersonBruntonPoseTrajectory2022: add "PetersonBruntonPoseTrajectory2022" to one of the dataset grouping lists used to build __all__ (e.g., add it to _ieeg_datasets alongside "Neuroprobe2025") so that __all__ (constructed from _electrophysiology_datasets, _calcium_imaging_datasets, _ieeg_datasets, _psg_datasets) includes PetersonBruntonPoseTrajectory2022; update the appropriate list declaration (the _ieeg_datasets list) so the class imported at the bottom (PetersonBruntonPoseTrajectory2022) is exported by from brainsets.datasets import *.
🧹 Nitpick comments (12)
pyproject.toml (1)
48-48: Defer pinningtorch_brainto commit SHA until upstream PR#173is released.The
@mainpin is currently necessary to track unreleased changes fromneuro-galaxy/torch_brain#173(specifically, theMultiChannelDatasetMixinAPI used inNeuroprobe2025.py). Once that upstream PR is merged and a new release is published, pin to an immutable commit SHA or tag to stabilize dev environments and improve reproducibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 48, Currently you're pinning torch_brain to `@main` to pull unreleased changes needed by MultiChannelDatasetMixin used in Neuroprobe2025.py; keep the dependency as the branch reference for now, and after upstream PR `#173` is merged and a new release is published, update the pyproject.toml entry "torch_brain@git+https://github.com/neuro-galaxy/torch_brain@main" to point to an immutable release tag or commit SHA (e.g., replace `@main` with @<commit-sha-or-tag>) to stabilize dev environments and improve reproducibility.brainsets/utils/split.py (2)
230-231: Rename ambiguous variableland tightenzip.Ruff flags
las an ambiguous name (E741, easily confused with1/I) andzipas missingstrict=(B905). Both trivially addressed:🧹 Proposed fix
- unique_labels, counts = np.unique(task_labels, return_counts=True) - undersized = {l: int(c) for l, c in zip(unique_labels, counts) if c < n_folds} + unique_labels, counts = np.unique(task_labels, return_counts=True) + undersized = { + label: int(count) + for label, count in zip(unique_labels, counts, strict=True) + if count < n_folds + }The similar
zipon line 78 can take the samestrict=Truefor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/split.py` around lines 230 - 231, Rename the ambiguous loop variable `l` to a clearer name like `label` in the comprehension that builds `undersized` (undersized = {label: int(c) for label, c in zip(unique_labels, counts) if c < n_folds}) and add strict=True to the zip call (zip(unique_labels, counts, strict=True)) to satisfy B905; also update the similar zip usage earlier in the file (the zip on or around the other comprehension at line ~78) to use strict=True for consistency.
201-252: Add a docstring to the new public helper.Every other public helper in this module has a rich docstring describing parameters, behavior, and error cases.
generate_stratified_folds_by_taskhas none, which makes the contract (keying scheme{task}_fold_{k}_{train|valid|test}, silent task-skipping behavior, required attribute, etc.) hard to discover for downstream pipeline authors.📝 Suggested docstring outline
def generate_stratified_folds_by_task( trials: Interval, task_configs: dict[str, list[str]], label_field: str, n_folds: int = 5, val_ratio: float = 0.2, seed: int = 42, ) -> dict[str, Interval]: """Generate stratified k-fold splits per task. For each entry in ``task_configs`` (``task_name -> included label values``), selects the trials whose ``label_field`` is in the included set and delegates to :func:`generate_stratified_folds`. Tasks with fewer than ``n_folds`` total trials, or any label category with fewer than ``n_folds`` samples, are skipped with a warning. Returns a flat mapping keyed by ``f"{task_name}_fold_{k}_{train|valid|test}"``. Raises: ValueError: If ``trials`` does not have ``label_field``. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/split.py` around lines 201 - 252, Add a comprehensive docstring to the public function generate_stratified_folds_by_task describing its purpose (generate stratified k-fold splits per task), parameters (trials, task_configs, label_field, n_folds, val_ratio, seed), return type (flat dict keyed by "{task_name}_fold_{k}_{train|valid|test}"), behavior (selects trials by label_field membership, delegates to generate_stratified_folds, skips tasks with fewer than n_folds trials or any label category with fewer than n_folds and logs a warning), and raised errors (ValueError if trials lacks label_field); ensure the docstring matches the style used by other helpers in this module and mentions the silent skipping behavior and keying scheme.tests/test_split_utils.py (1)
497-497: Nit: use attribute access instead ofgetattrwith a constant.Ruff flags B009. Since the attribute name is a string literal, direct access is clearer and equivalent.
🧹 Proposed fix
- split_labels = set(np.asarray(getattr(split, "behavior_labels"))) + split_labels = set(np.asarray(split.behavior_labels))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_split_utils.py` at line 497, Replace the getattr call with direct attribute access: change the expression using getattr(split, "behavior_labels") to use split.behavior_labels instead (keep the surrounding np.asarray(...) and set(...) logic intact); this removes the unnecessary getattr usage flagged by Ruff (B009) and keeps behavior_labels access consistent in tests/test_split_utils.py where variable split is used.tests/test_dandi_utils.py (1)
1-138: Test coverage reads well; consider one extra edge case.The SimpleNamespace +
FakeElectrodesharness matches the attribute-access pattern indandi_utils.pynicely, and covers the key branches (override vs. inferred, ambiguous → subject fallback, missingElectricalSeries).Optional suggestion: once the operator-precedence issue flagged in
dandi_utils.py(_hemisphere_from_nwb) is fixed, add a test withlocation=["l", "l", "l"](single-character values). That case currently silently falls through toUNKNOWNand a test would lock in the fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_dandi_utils.py` around lines 1 - 138, The _hemisphere_from_nwb function has an operator-precedence/logic bug that causes single-character location strings like "l" or "r" to fall through to UNKNOWN; update _hemisphere_from_nwb to normalize the location and subject_hemisphere to lowercase/stripped values and use clear checks (e.g., exact match for "l"/"r" or startswith checks for full words like "left"/"right") with proper parentheses/ordering so single-character values are recognized, and ensure the function still prefers an explicit subject_hemisphere parameter when provided.brainsets/utils/dandi_utils.py (1)
249-250: Drop the unusedn_channelsparameter.
_hemisphere_from_nwbacceptsn_channelsbut never uses it; the body only iterates over electrode column values.extract_ecog_from_nwbpasses it on line 319, so remove from both places to avoid a misleading signature.♻️ Proposed fix
-def _hemisphere_from_nwb(nwbfile: NWBFile, n_channels: int) -> Hemisphere: +def _hemisphere_from_nwb(nwbfile: NWBFile) -> Hemisphere: colnames = getattr(nwbfile.electrodes, "colnames", [])And at the call site:
- hemisphere = _hemisphere_from_nwb(nwbfile, n_channels) + hemisphere = _hemisphere_from_nwb(nwbfile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/dandi_utils.py` around lines 249 - 250, The _hemisphere_from_nwb function currently has an unused parameter n_channels; remove n_channels from the _hemisphere_from_nwb signature and update every call site to stop passing that argument (notably where extract_ecog_from_nwb forwards it), and then run tests/lint to ensure no remaining references; keep the function body unchanged and only adjust the signature and callers (search for _hemisphere_from_nwb and remove the extra argument in extract_ecog_from_nwb and any other invocations).brainsets/datasets/PetersonBruntonPoseTrajectory2022.py (1)
12-12: Duplicated label list between pipeline and dataset.
BEHAVIOR_LABELShere duplicatesBEHAVIOR_TASK_CONFIGS["all_active_behavior"]inbrainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py. If the task config ever drifts (e.g., adding "Other activity" back), intrasession splits and intersubject/intersession filtering will silently disagree. Consider centralizing the canonical label list in a shared module that both sides import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py` at line 12, The dataset defines a local BEHAVIOR_LABELS list that duplicates the canonical labels in BEHAVIOR_TASK_CONFIGS["all_active_behavior"], which can cause silent mismatches; refactor to import the canonical list instead of duplicating it: remove or replace BEHAVIOR_LABELS in PetersonBruntonPoseTrajectory2022 with an import (or accessor) that references the shared source (the BEHAVIOR_TASK_CONFIGS entry) so both the dataset and the pipeline use the same symbol, and add a brief comment noting the canonical source to prevent future drift.brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (4)
139-146: Parameter namepose_valid_domainis misleading — caller passes the joint signal valid domain.
process()invokesself._generate_splits(..., signal_valid_domain)(line 351), which is the intersection of pose and ECoG validity. Rename the parameter to avoid suggesting it is pose-only; this is consistent with how it's stored inData(pose_valid_domain=signal_valid_domain, …)at line 375 (also worth renaming that attribute for symmetry with the dataset-sidedata.pose_valid_domainusage).Also applies to: 345-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 139 - 146, The parameter name pose_valid_domain in _generate_splits is misleading because callers pass the joint signal validity (pose ∩ ECoG); rename the parameter to signal_valid_domain (or joint_signal_valid_domain) in the _generate_splits signature and all its call sites (e.g., where process() calls _generate_splits with signal_valid_domain) and update the Data construction that currently uses Data(pose_valid_domain=signal_valid_domain, …) to Data(signal_valid_domain=signal_valid_domain, …) (also rename the Data attribute and any downstream references such as data.pose_valid_domain to data.signal_valid_domain) so names consistently reflect that this interval covers joint signal validity rather than pose-only.
276-276: Replacegetattron a constant attribute with direct access.
getattr(self.args, "resample_rate")(Ruff B009) is equivalent toself.args.resample_rateand no safer. Same pattern appears at lines 134 (redownload) and 262 (reprocess), where a default is supplied — those are fine. Only this one has no default and should use attribute access.- resample_rate = getattr(self.args, "resample_rate") + resample_rate = self.args.resample_rate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` at line 276, Replace the unsafe getattr call by directly accessing the attribute: change getattr(self.args, "resample_rate") to self.args.resample_rate (keep other occurrences with defaults at the earlier lines unchanged); locate the call to getattr on self.args for resample_rate in the pipeline code and use direct attribute access instead.
449-452: Preserve exception chain when re-raisingImportError.Ruff B904. Use
raise ... from err(orfrom Noneif you want to swallow the original).- try: - from scipy import signal - except ImportError: - raise ImportError("resample_ecog_ajile requires scipy") + try: + from scipy import signal + except ImportError as err: + raise ImportError("resample_ecog_ajile requires scipy") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 449 - 452, The ImportError for the scipy import should preserve the original exception chain; in the try/except around "from scipy import signal" capture the exception (e.g., except ImportError as err) and re-raise with "raise ImportError('resample_ecog_ajile requires scipy') from err" so the original traceback is preserved.
233-265: Skip-existing check opens the NWB file unnecessarily.The early-exit at lines 262–265 depends on
session_id, which requiresnwbfile.session_start_timeand therefore forcesio.read()even when the output already exists. Since the manifest already knowssubject,session_num, and the raw file path, you can derivesession_idfrom filename + a cachedsession_start_time(or just test for any existingf"AJILE12_P{subject_num}_*_ses{session_num}_pose_trajectories.h5"viaglob) and avoid a costly NWB open on the re-run path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 233 - 265, Skip the expensive NWB open by moving io = NWBHDF5IO(...) and nwbfile = io.read() after the early-exit check: parse subject_num and session_num from the raw filename/stem (look for "sub-" or "AJILE12_P" patterns used in subject.id logic) and build a glob pattern like f"AJILE12_P{subject_num}_*_ses{session_num}_pose_trajectories.h5" against self.processed_dir; if a match exists (or output_path exists) and not reprocessing, return before calling NWBHDF5IO.read(); only when no existing output is found, open the NWB (nwbfile) to get session_start_time and construct session_id/output_path exactly as before (variables: process(), fpath, stem, subject_num, session_num, processed_dir, output_path, io, nwbfile, session_id).docs/source/glossary/brainsets.rst (1)
713-718: Citation block inconsistent with sibling entries.All other brainsets in this file include a
<span class="citation-container">with BibTeX/APA popups and Cite buttons. This new entry only exposes a bare DOI link, so users cannot copy a citation from the UI the way they can for every other dataset.Also worth noting (skip if intentional): no "Distribution of recording lengths" histogram row, and "Processed data size" is
TBA— presumably placeholders until the processed artifacts are generated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/glossary/brainsets.rst` around lines 713 - 718, The Publication(s) cell currently contains only a bare DOI link; wrap that DOI link in the same citation UI used by sibling entries by adding a <span class="citation-container"> around the anchor and including the same BibTeX/APA popup and cite button markup used elsewhere in this file (match the pattern used in other <tr> entries for "Publication(s)"), and also verify the presence of the "Distribution of recording lengths" histogram row and replace the "Processed data size" TBA placeholder with the actual size (or explicitly mark as intentionally pending) so this entry matches the structure of sibling brainset rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/participants.json`:
- Around line 1-14: Add a short source reference to participants.json stating
that these metadata values come from the AJILE12 dataset (DANDI:000055, release
0.220127.0436) and the Peterson et al. 2022 paper by either (preferred)
inserting a top-level "_comment" field in the JSON (above keys "01", "02", ...)
with the DANDI URL and citation, or by adding a sibling README (e.g.,
participants.README) that names the source, DANDI identifier/URL, release tag,
and the paper reference so maintainers can re-verify the
"age"/"sex"/"hemi"/"days" entries against the original participants.tsv.
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 58-62: The CLI defines parser.add_argument("--filter_ecog",
action="store_true", help="Apply 70–150 Hz bandpass and Hilbert envelope") but
the flag is unused; either remove this argument or wire it into the ECoG
preprocessing path by checking self.args.filter_ecog where ECoG signals are
handled (e.g., in the function/method that performs ECoG preprocessing or
feature extraction) and apply the 70-150 Hz bandpass + Hilbert envelope when
true; also change the help string to use an ASCII hyphen ("70-150 Hz") to
replace the en dash. Ensure you update or remove any references/tests that
expect the flag accordingly.
- Around line 485-521: The function ajile_extract_pose_from_nwb currently
declares a return type Tuple[Interval, RegularTimeSeries] but only returns a
single RegularTimeSeries instance; update the signature to return Just
RegularTimeSeries (i.e., change the annotation to -> RegularTimeSeries) so type
checkers and callers match the actual return value, and run static/type checks
to ensure no other code expects a tuple from ajile_extract_pose_from_nwb.
- Around line 327-343: The log line can raise ZeroDivisionError when
original_avi_duration is 0.0; update the block around
active_vs_inactive_trials/_trim_trials_to_domain so you compute percent_removed
defensively (e.g., if original_avi_duration > 0 then percent = 100 *
(original_avi_duration - trimmed_avi_duration) / original_avi_duration else
percent = 0.0 or "N/A") and pass that safe value into logging.info; ensure you
still compute original_avi_duration and trimmed_avi_duration as before and only
avoid the division when original_avi_duration is zero to prevent aborts.
- Around line 280-296: The code has inconsistent RegularTimeSeries field names
causing runtime AttributeError: standardize on "signal" everywhere; update
resample_ecog_ajile to read ecog_rts.signal[:] instead of ecog_rts.ecogs and to
construct RegularTimeSeries(signal=data_out, ...), and verify
extract_ecog_from_nwb still produces RegularTimeSeries(signal=...), and ensure
compute_ecog_valid_domain continues to access ecog.signal[:]; update any other
uses of "ecogs" to "signal" so all functions (extract_ecog_from_nwb,
resample_ecog_ajile, compute_ecog_valid_domain and the RegularTimeSeries
constructors) use the same attribute name.
- Around line 167-193: Subdivide() drops auxiliary attributes so rebuild
per-chunk Interval objects before calling generate_stratified_folds_by_task():
after calling Interval.subdivide() for behavior_chunks and avi_chunks, iterate
chunks and construct new Interval-like objects using each chunk's start/end and
copy over the required attributes (behavior_labels, behavior_id, timestamps)
from the original trial intervals (same pattern used later in the file), then
pass those reconstructed chunk lists into generate_stratified_folds_by_task()
(functions/classes referenced: Interval.subdivide,
generate_stratified_folds_by_task, variables behavior_chunks, avi_chunks, and
attributes behavior_labels, behavior_id, timestamps).
In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py`:
- Around line 19-25: The intersubject/intersession path returns
data.active_behavior_trials unfiltered, causing "Other activity" to be included;
update that branch to use the existing helper
_behavior_trials_for_task(recording, task_type) (or call it with
task_type="behavior" / "active_vs_inactive") instead of directly returning
data.active_behavior_trials so the trials are filtered by BEHAVIOR_LABELS (and
apply the same change for the symmetric active_vs_inactive branch); locate
references to data.active_behavior_trials in the intersubject/intersession
handling and replace with a call to _behavior_trials_for_task(recording,
task_type).
In `@brainsets/utils/dandi_utils.py`:
- Around line 258-267: The boolean expressions computing left and right have an
operator-precedence bug: bitwise OR (|) binds tighter than comparison, causing
(texts == "l") / (texts == "r") to be dropped when OR'ed with np.char.find
results; fix by adding explicit parentheses around each equality and the ORed
equality terms, e.g. wrap (texts == "l") and (texts == "left") together and
similarly for the "r" branch so the expressions using
np.char.find(texts.astype(str), "left") >= 0 and >= 0 are combined correctly;
update the variables left and right in the same block that references texts and
np.char.find to ensure correct grouping.
In `@CHANGELOG.md`:
- Line 17: The changelog entry for `peterson_brunton_pose_trajectory_2022` is an
incomplete sentence with a dangling "(AJILE12)"; update the line to fully
describe what was added (e.g., dataset class and brainset/pipeline) to match the
style of neighboring entries like `Neuroprobe2025`. Replace the current fragment
"- Added the `peterson_brunton_pose_trajectory_2022` (AJILE12)." with a complete
phrase that names the dataset class and the brainset/pipeline, e.g. "Added the
`peterson_brunton_pose_trajectory_2022` dataset class and AJILE12
brainset/pipeline." ensuring the entry is self-contained and parallel to other
entries.
---
Outside diff comments:
In `@brainsets/datasets/__init__.py`:
- Around line 14-38: The export list is missing
PetersonBruntonPoseTrajectory2022: add "PetersonBruntonPoseTrajectory2022" to
one of the dataset grouping lists used to build __all__ (e.g., add it to
_ieeg_datasets alongside "Neuroprobe2025") so that __all__ (constructed from
_electrophysiology_datasets, _calcium_imaging_datasets, _ieeg_datasets,
_psg_datasets) includes PetersonBruntonPoseTrajectory2022; update the
appropriate list declaration (the _ieeg_datasets list) so the class imported at
the bottom (PetersonBruntonPoseTrajectory2022) is exported by from
brainsets.datasets import *.
In `@brainsets/utils/split.py`:
- Around line 1-6: The module's public export list misses the new helper: add
"generate_stratified_folds_by_task" to the _functions list (and thus __all__)
alongside "generate_stratified_folds" and "generate_string_kfold_assignment" so
the function is exported; update the _functions array in the top-level of
brainsets/utils/split.py to include the exact symbol name
generate_stratified_folds_by_task.
---
Nitpick comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 139-146: The parameter name pose_valid_domain in _generate_splits
is misleading because callers pass the joint signal validity (pose ∩ ECoG);
rename the parameter to signal_valid_domain (or joint_signal_valid_domain) in
the _generate_splits signature and all its call sites (e.g., where process()
calls _generate_splits with signal_valid_domain) and update the Data
construction that currently uses Data(pose_valid_domain=signal_valid_domain, …)
to Data(signal_valid_domain=signal_valid_domain, …) (also rename the Data
attribute and any downstream references such as data.pose_valid_domain to
data.signal_valid_domain) so names consistently reflect that this interval
covers joint signal validity rather than pose-only.
- Line 276: Replace the unsafe getattr call by directly accessing the attribute:
change getattr(self.args, "resample_rate") to self.args.resample_rate (keep
other occurrences with defaults at the earlier lines unchanged); locate the call
to getattr on self.args for resample_rate in the pipeline code and use direct
attribute access instead.
- Around line 449-452: The ImportError for the scipy import should preserve the
original exception chain; in the try/except around "from scipy import signal"
capture the exception (e.g., except ImportError as err) and re-raise with "raise
ImportError('resample_ecog_ajile requires scipy') from err" so the original
traceback is preserved.
- Around line 233-265: Skip the expensive NWB open by moving io = NWBHDF5IO(...)
and nwbfile = io.read() after the early-exit check: parse subject_num and
session_num from the raw filename/stem (look for "sub-" or "AJILE12_P" patterns
used in subject.id logic) and build a glob pattern like
f"AJILE12_P{subject_num}_*_ses{session_num}_pose_trajectories.h5" against
self.processed_dir; if a match exists (or output_path exists) and not
reprocessing, return before calling NWBHDF5IO.read(); only when no existing
output is found, open the NWB (nwbfile) to get session_start_time and construct
session_id/output_path exactly as before (variables: process(), fpath, stem,
subject_num, session_num, processed_dir, output_path, io, nwbfile, session_id).
In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py`:
- Line 12: The dataset defines a local BEHAVIOR_LABELS list that duplicates the
canonical labels in BEHAVIOR_TASK_CONFIGS["all_active_behavior"], which can
cause silent mismatches; refactor to import the canonical list instead of
duplicating it: remove or replace BEHAVIOR_LABELS in
PetersonBruntonPoseTrajectory2022 with an import (or accessor) that references
the shared source (the BEHAVIOR_TASK_CONFIGS entry) so both the dataset and the
pipeline use the same symbol, and add a brief comment noting the canonical
source to prevent future drift.
In `@brainsets/utils/dandi_utils.py`:
- Around line 249-250: The _hemisphere_from_nwb function currently has an unused
parameter n_channels; remove n_channels from the _hemisphere_from_nwb signature
and update every call site to stop passing that argument (notably where
extract_ecog_from_nwb forwards it), and then run tests/lint to ensure no
remaining references; keep the function body unchanged and only adjust the
signature and callers (search for _hemisphere_from_nwb and remove the extra
argument in extract_ecog_from_nwb and any other invocations).
In `@brainsets/utils/split.py`:
- Around line 230-231: Rename the ambiguous loop variable `l` to a clearer name
like `label` in the comprehension that builds `undersized` (undersized = {label:
int(c) for label, c in zip(unique_labels, counts) if c < n_folds}) and add
strict=True to the zip call (zip(unique_labels, counts, strict=True)) to satisfy
B905; also update the similar zip usage earlier in the file (the zip on or
around the other comprehension at line ~78) to use strict=True for consistency.
- Around line 201-252: Add a comprehensive docstring to the public function
generate_stratified_folds_by_task describing its purpose (generate stratified
k-fold splits per task), parameters (trials, task_configs, label_field, n_folds,
val_ratio, seed), return type (flat dict keyed by
"{task_name}_fold_{k}_{train|valid|test}"), behavior (selects trials by
label_field membership, delegates to generate_stratified_folds, skips tasks with
fewer than n_folds trials or any label category with fewer than n_folds and logs
a warning), and raised errors (ValueError if trials lacks label_field); ensure
the docstring matches the style used by other helpers in this module and
mentions the silent skipping behavior and keying scheme.
In `@docs/source/glossary/brainsets.rst`:
- Around line 713-718: The Publication(s) cell currently contains only a bare
DOI link; wrap that DOI link in the same citation UI used by sibling entries by
adding a <span class="citation-container"> around the anchor and including the
same BibTeX/APA popup and cite button markup used elsewhere in this file (match
the pattern used in other <tr> entries for "Publication(s)"), and also verify
the presence of the "Distribution of recording lengths" histogram row and
replace the "Processed data size" TBA placeholder with the actual size (or
explicitly mark as intentionally pending) so this entry matches the structure of
sibling brainset rows.
In `@pyproject.toml`:
- Line 48: Currently you're pinning torch_brain to `@main` to pull unreleased
changes needed by MultiChannelDatasetMixin used in Neuroprobe2025.py; keep the
dependency as the branch reference for now, and after upstream PR `#173` is merged
and a new release is published, update the pyproject.toml entry
"torch_brain@git+https://github.com/neuro-galaxy/torch_brain@main" to point to
an immutable release tag or commit SHA (e.g., replace `@main` with
@<commit-sha-or-tag>) to stabilize dev environments and improve reproducibility.
In `@tests/test_dandi_utils.py`:
- Around line 1-138: The _hemisphere_from_nwb function has an
operator-precedence/logic bug that causes single-character location strings like
"l" or "r" to fall through to UNKNOWN; update _hemisphere_from_nwb to normalize
the location and subject_hemisphere to lowercase/stripped values and use clear
checks (e.g., exact match for "l"/"r" or startswith checks for full words like
"left"/"right") with proper parentheses/ordering so single-character values are
recognized, and ensure the function still prefers an explicit subject_hemisphere
parameter when provided.
In `@tests/test_split_utils.py`:
- Line 497: Replace the getattr call with direct attribute access: change the
expression using getattr(split, "behavior_labels") to use split.behavior_labels
instead (keep the surrounding np.asarray(...) and set(...) logic intact); this
removes the unnecessary getattr usage flagged by Ruff (B009) and keeps
behavior_labels access consistent in tests/test_split_utils.py where variable
split is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 375be068-fda6-4f00-a1e5-10e8d42d7df7
📒 Files selected for processing (14)
.github/workflows/pipeline_testing.ymlCHANGELOG.mdREADME.mdbrainsets/datasets/PetersonBruntonPoseTrajectory2022.pybrainsets/datasets/__init__.pybrainsets/utils/dandi_utils.pybrainsets/utils/split.pybrainsets_pipelines/peterson_brunton_pose_trajectory_2022/participants.jsonbrainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.pydocs/source/glossary/brainsets.rstpyproject.tomltests/test_cli.pytests/test_dandi_utils.pytests/test_split_utils.py
💤 Files with no reviewable changes (1)
- tests/test_cli.py
milosobral
added
the
new brainset
… documentation, added ECoG extraction utilities, and improved stratified splitting functionality. Update CHANGELOG to reflect these changes. Fix minor issues in pipeline testing workflow.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_split_utils.py (1)
1-560:⚠️ Potential issue | 🟡 MinorRun
blackto resolve the linting pipeline failure.GitHub Actions reports this file needs reformatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_split_utils.py` around lines 1 - 560, The file tests/test_split_utils.py is failing the formatting check; run the project's formatter (black) on this file (or the repository) to reformat it to the style expected by CI (e.g., run `black tests/test_split_utils.py`), then add the reformatted file to the commit; you can verify by rerunning lint/CI—look at test classes like TestGenerateStratifiedFolds, TestGenerateStringKfoldAssignment, and TestGenerateStratifiedFoldsByTask to locate the file in the repo.
♻️ Duplicate comments (3)
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (2)
199-225:⚠️ Potential issue | 🔴 Critical
Interval.subdivide(...)likely dropsbehavior_labels→generate_stratified_folds_by_taskwill raise.
behavior_chunks/avi_chunksare built via.subdivide(...), andgenerate_stratified_folds_by_taskimmediately requires abehavior_labelsattribute (brainsets/utils/split.pyL231-234). If subdivide doesn't propagate auxiliary attrs (as was the case historically), this code path raises on any session with behavior. No test currently exercises this subdivide→task-split integration, so CI won't catch it.The same pattern is reconstructed manually in
_trim_trials_to_domain(L583-593). Apply it here: aftersubdivide, remap chunk-start → parent-trial index (e.g.,np.searchsorted(trials.start, chunks.start, side='right') - 1, withside/bounds care) and reattachbehavior_labels/behavior_id/timestampson the chunks before calling the task splitter.#!/bin/bash # Confirm whether Interval.subdivide preserves auxiliary attributes in the pinned temporaldata version. python - <<'PY' import numpy as np from temporaldata import Interval iv = Interval( start=np.array([0., 10., 20.], dtype=float), end=np.array([9., 19., 29.], dtype=float), behavior_labels=np.array(["Eat","Talk","TV"]), ) sub = iv.subdivide(step=3.0, drop_short=False) print("len:", len(sub), "has behavior_labels:", hasattr(sub, "behavior_labels")) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 199 - 225, behavior_chunks and avi_chunks created by Interval.subdivide may not preserve auxiliary attributes (behavior_labels, behavior_id, timestamps), causing generate_stratified_folds_by_task to fail; after subdivide() on active_behavior_trials and active_vs_inactive_trials, map each chunk back to its parent trial using the parent trial starts (e.g., parent_idx = np.searchsorted(trials.start, chunks.start, side="right") - 1) and then reattach behavior_labels, behavior_id, and timestamps from the original Interval to the chunks (same approach used in _trim_trials_to_domain around the block that remaps attributes) before calling generate_stratified_folds_by_task for both behavior_chunks and avi_chunks.
375-379:⚠️ Potential issue | 🟡 Minor
ZeroDivisionErrorwhenoriginal_avi_duration == 0.0.If the session has zero-duration or all-clipped active-vs-inactive trials, the log-line divide by
original_avi_durationaborts processing. Guard with a conditional.Proposed fix
- logger.info( - f"Trimmed active_vs_inactive trials to valid signal domain: " - f"{original_avi_duration:.1f}s -> {trimmed_avi_duration:.1f}s " - f"({100 * (original_avi_duration - trimmed_avi_duration) / original_avi_duration:.1f}% removed)." - ) + pct_removed = ( + 100.0 * (original_avi_duration - trimmed_avi_duration) / original_avi_duration + if original_avi_duration > 0 + else 0.0 + ) + logger.info( + f"Trimmed active_vs_inactive trials to valid signal domain: " + f"{original_avi_duration:.1f}s -> {trimmed_avi_duration:.1f}s " + f"({pct_removed:.1f}% removed)." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 375 - 379, The log formatting in the logger.info call that computes percent removed can raise ZeroDivisionError when original_avi_duration == 0.0; update the code around the logger.info that references original_avi_duration and trimmed_avi_duration to guard the division (e.g., compute percent_removed only if original_avi_duration > 0, otherwise use 0.0 or "N/A") and use that safe percent_removed value in the log message so no division by zero can occur.brainsets/datasets/PetersonBruntonPoseTrajectory2022.py (1)
185-201:⚠️ Potential issue | 🟠 MajorLabel set asymmetry between intrasession and intersubject/intersession for
task_type="behavior".Intrasession
behaviorreadssplits.all_active_behavior_fold_*, which the pipeline populates fromBEHAVIOR_TASK_CONFIGS["all_active_behavior"] = ["Eat","Talk","TV","Computer/phone"]—"Other activity"is excluded. The intersubject/intersession branch here returnsdata.active_behavior_trialsdirectly, which still contains"Other activity"(see pipelineACTIVE_BEHAVIOR_PRIORITYL61 andactive_behavior_trialsconstruction L676-682). The two split types therefore expose different label sets for the sametask_type, which will skew cross-split evaluations.Either filter
active_behavior_trialsby the same label set before returning, or (preferably) have the pipeline also write a filteredactive_behavior_trials_for_all_active_behaviorattribute and read it here, so both paths share a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py` around lines 185 - 201, The behavior labels are asymmetric: when task_type == "behavior" the intrasession path uses the pipeline's BEHAVIOR_TASK_CONFIGS["all_active_behavior"] set (which excludes "Other activity") but the intersubject/intersession branch returns data.active_behavior_trials (which still contains "Other activity"); update the intersubject/intersession branch in the function handling task_type (the block that references self.task_type == "behavior" and data.active_behavior_trials) to either filter data.active_behavior_trials to only include labels from BEHAVIOR_TASK_CONFIGS["all_active_behavior"] before assigning result[rid], or (preferred) read a new attribute data.active_behavior_trials_for_all_active_behavior (to be written by the pipeline) instead of data.active_behavior_trials so both paths use the same label set.
🧹 Nitpick comments (5)
brainsets/datasets/PetersonBruntonPoseTrajectory2022.py (1)
99-113: Minor: validatefold_numbertype explicitly.
0 <= fold_number < N_FOLDSaccepts e.g.0.5orTrue. If you care about this, addisinstance(fold_number, int)or cast viaoperator.index. Optional; ignore if you rely on Python's typing contracts externally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py` around lines 99 - 113, The current validation for fold_number only checks numeric range and will accept non-integer types (e.g., 0.5 or True); update the check around fold_number (used with N_FOLDS) to explicitly ensure an integer-like value by either using isinstance(fold_number, int) or calling operator.index(fold_number) (catching TypeError/ValueError) before the range check, and raise the same ValueError message if it fails; leave the existing VALID_SPLIT_TYPES and VALID_TASK_TYPES checks unchanged.brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (1)
408-414:pose_valid_domainattribute is actually the joint signal-valid domain.You store
pose_valid_domain=signal_valid_domain(the ECoG ∩ pose intersection). The dataset class readsdata.pose_valid_domainas the pose-estimation sampling domain (intersubject/intersession path). If that's intentional (pose-estimation should only be evaluated where ECoG is also valid), a one-line comment here and/or renaming either the attribute or the variable would prevent future confusion. Otherwise, you likely want to store the pure pose domain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 408 - 414, The code assigns pose_valid_domain=signal_valid_domain (i.e., the ECoG ∩ pose intersection) but the dataset expects data.pose_valid_domain to represent the pure pose-estimation sampling domain; fix this by either passing the pure pose domain (rename/use the variable that holds only the pose mask) into the call instead of signal_valid_domain for the pose_valid_domain parameter, or, if intentional, add a one-line clarifying comment and/or rename the parameter/variable (e.g., pose_valid_domain_from_signal or joint_pose_ecog_domain) to make the intersection intent explicit; update references to pose_valid_domain and signal_valid_domain in the surrounding code to match the chosen approach.brainsets/utils/split.py (1)
80-86: Behavior change: undersized categories now hard-fail.Previously
generate_stratified_foldswould delegate the category-count check to sklearn'sStratifiedKFold(which produces a clearer but later error). Hard-failing upfront is reasonable, but note this is a breaking change for any existing direct caller that was relying on the prior (more permissive) behavior.generate_stratified_folds_by_taskpre-filters and skips so it's unaffected. Consider mentioning this inCHANGELOG.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/split.py` around lines 80 - 86, The new upfront validation in generate_stratified_folds now raises ValueError for categories with fewer than n_folds (instead of letting sklearn.StratifiedKFold raise later), which is a breaking behavior change for callers; update CHANGELOG.md to document this breaking change and cite the affected function generate_stratified_folds (note that generate_stratified_folds_by_task remains unchanged because it pre-filters), include the exact symptom (undersized categories now hard-fail with ValueError) and guidance for users (either ensure category counts >= n_folds or use pre-filtering like generate_stratified_folds_by_task).tests/test_dandi_utils.py (1)
192-200: The validator behavior is correct; consider adding an explicit unit test forSubjectDescription.normalize_speciesto isolate the coercion contract.The
normalize_speciesfield validator correctly coerces unrecognized species strings (viaSpecies.from_string()catchingValueError) toSpecies.UNKNOWN, as your comment states. The test correctly validates this behavior through integration testing. However, the contract would be more explicit if the validator were unit-tested directly—either by testingSubjectDescription's field behavior in isolation or by adding a focused test that calls the validator method directly. This guards against future changes to the validator that might silently break the implicit assumption in this integration test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_dandi_utils.py` around lines 192 - 200, Add a focused unit test that calls the SubjectDescription.normalize_species validator directly to assert it coerces unknown strings to Species.UNKNOWN; specifically, invoke SubjectDescription.normalize_species (or call Species.from_string and simulate the ValueError path) with an unrecognized string like "Alien species" and assert the return is Species.UNKNOWN to make the coercion contract explicit and independent of extract_subject_from_nwb.tests/test_split_utils.py (1)
455-560: Add an integration test forInterval.subdivide()→generate_stratified_folds_by_taskworkflow.The existing tests construct
trialswithbehavior_labelsset directly. The pipeline (pipeline.py L201–204, L215–218) instead passessubdivided_intervaldirectly togenerate_stratified_folds_by_taskwithlabel_field="behavior_labels". Ifsubdivide()does not preserve auxiliary attributes, this integration will fail withValueError: Trials must have a 'behavior_labels' attribute. A test that subdivides anIntervalwithbehavior_labelsand then passes it togenerate_stratified_folds_by_taskwould catch such regressions and validate the integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_split_utils.py` around lines 455 - 560, Add an integration test that constructs an Interval with behavior_labels, calls Interval.subdivide(...) to produce subdivided_interval, then calls generate_stratified_folds_by_task(trials=subdivided_interval, label_field="behavior_labels", ...) and asserts it returns the expected splits (e.g., non-empty dict and correct keys) so we detect if Interval.subdivide fails to preserve auxiliary attributes; target the Interval.subdivide and generate_stratified_folds_by_task symbols when locating where to add the test in tests/test_split_utils.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 1-706: The file fails the linter because it is not formatted with
Black; run Black against this file (or the repo) to apply the expected
formatting changes so definitions like class Pipeline, functions
resample_ecog_ajile, ajile_extract_pose_from_nwb, compute_pose_valid_domain, and
ajile_extract_behavior_intervals_from_nwb comply with the project's Black
config; commit the reformatted file to resolve the GitHub Actions formatting
check.
- Around line 289-291: Replace the runtime-conditional assert used for input
validation with an explicit exception: instead of "assert subject_num and
subject_num.isdigit()", raise a ValueError when subject_num is falsy or
non-numeric and include the same message referencing subject.id so failures are
deterministic (e.g., raise ValueError(f"Could not parse numeric subject from id
'{subject.id}'")). This ensures invalid subject_num is caught regardless of -O
optimization and prevents malformed session_id downstream.
In `@tests/test_dandi_utils.py`:
- Around line 1-207: The file fails formatting checks — run the project's Black
formatter on the test module (the file containing TestExtractSubjectFromNwb,
TestExtractEcogFromNwb, and TestNormalizeSubjectSpecies) to fix style issues;
apply Black (e.g., black <that file>) and commit the reformatted file so CI
linting passes.
---
Outside diff comments:
In `@tests/test_split_utils.py`:
- Around line 1-560: The file tests/test_split_utils.py is failing the
formatting check; run the project's formatter (black) on this file (or the
repository) to reformat it to the style expected by CI (e.g., run `black
tests/test_split_utils.py`), then add the reformatted file to the commit; you
can verify by rerunning lint/CI—look at test classes like
TestGenerateStratifiedFolds, TestGenerateStringKfoldAssignment, and
TestGenerateStratifiedFoldsByTask to locate the file in the repo.
---
Duplicate comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 199-225: behavior_chunks and avi_chunks created by
Interval.subdivide may not preserve auxiliary attributes (behavior_labels,
behavior_id, timestamps), causing generate_stratified_folds_by_task to fail;
after subdivide() on active_behavior_trials and active_vs_inactive_trials, map
each chunk back to its parent trial using the parent trial starts (e.g.,
parent_idx = np.searchsorted(trials.start, chunks.start, side="right") - 1) and
then reattach behavior_labels, behavior_id, and timestamps from the original
Interval to the chunks (same approach used in _trim_trials_to_domain around the
block that remaps attributes) before calling generate_stratified_folds_by_task
for both behavior_chunks and avi_chunks.
- Around line 375-379: The log formatting in the logger.info call that computes
percent removed can raise ZeroDivisionError when original_avi_duration == 0.0;
update the code around the logger.info that references original_avi_duration and
trimmed_avi_duration to guard the division (e.g., compute percent_removed only
if original_avi_duration > 0, otherwise use 0.0 or "N/A") and use that safe
percent_removed value in the log message so no division by zero can occur.
In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py`:
- Around line 185-201: The behavior labels are asymmetric: when task_type ==
"behavior" the intrasession path uses the pipeline's
BEHAVIOR_TASK_CONFIGS["all_active_behavior"] set (which excludes "Other
activity") but the intersubject/intersession branch returns
data.active_behavior_trials (which still contains "Other activity"); update the
intersubject/intersession branch in the function handling task_type (the block
that references self.task_type == "behavior" and data.active_behavior_trials) to
either filter data.active_behavior_trials to only include labels from
BEHAVIOR_TASK_CONFIGS["all_active_behavior"] before assigning result[rid], or
(preferred) read a new attribute
data.active_behavior_trials_for_all_active_behavior (to be written by the
pipeline) instead of data.active_behavior_trials so both paths use the same
label set.
---
Nitpick comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 408-414: The code assigns pose_valid_domain=signal_valid_domain
(i.e., the ECoG ∩ pose intersection) but the dataset expects
data.pose_valid_domain to represent the pure pose-estimation sampling domain;
fix this by either passing the pure pose domain (rename/use the variable that
holds only the pose mask) into the call instead of signal_valid_domain for the
pose_valid_domain parameter, or, if intentional, add a one-line clarifying
comment and/or rename the parameter/variable (e.g.,
pose_valid_domain_from_signal or joint_pose_ecog_domain) to make the
intersection intent explicit; update references to pose_valid_domain and
signal_valid_domain in the surrounding code to match the chosen approach.
In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py`:
- Around line 99-113: The current validation for fold_number only checks numeric
range and will accept non-integer types (e.g., 0.5 or True); update the check
around fold_number (used with N_FOLDS) to explicitly ensure an integer-like
value by either using isinstance(fold_number, int) or calling
operator.index(fold_number) (catching TypeError/ValueError) before the range
check, and raise the same ValueError message if it fails; leave the existing
VALID_SPLIT_TYPES and VALID_TASK_TYPES checks unchanged.
In `@brainsets/utils/split.py`:
- Around line 80-86: The new upfront validation in generate_stratified_folds now
raises ValueError for categories with fewer than n_folds (instead of letting
sklearn.StratifiedKFold raise later), which is a breaking behavior change for
callers; update CHANGELOG.md to document this breaking change and cite the
affected function generate_stratified_folds (note that
generate_stratified_folds_by_task remains unchanged because it pre-filters),
include the exact symptom (undersized categories now hard-fail with ValueError)
and guidance for users (either ensure category counts >= n_folds or use
pre-filtering like generate_stratified_folds_by_task).
In `@tests/test_dandi_utils.py`:
- Around line 192-200: Add a focused unit test that calls the
SubjectDescription.normalize_species validator directly to assert it coerces
unknown strings to Species.UNKNOWN; specifically, invoke
SubjectDescription.normalize_species (or call Species.from_string and simulate
the ValueError path) with an unrecognized string like "Alien species" and assert
the return is Species.UNKNOWN to make the coercion contract explicit and
independent of extract_subject_from_nwb.
In `@tests/test_split_utils.py`:
- Around line 455-560: Add an integration test that constructs an Interval with
behavior_labels, calls Interval.subdivide(...) to produce subdivided_interval,
then calls generate_stratified_folds_by_task(trials=subdivided_interval,
label_field="behavior_labels", ...) and asserts it returns the expected splits
(e.g., non-empty dict and correct keys) so we detect if Interval.subdivide fails
to preserve auxiliary attributes; target the Interval.subdivide and
generate_stratified_folds_by_task symbols when locating where to add the test in
tests/test_split_utils.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71bdbb2c-e2bb-43ac-b459-3802c237af98
📒 Files selected for processing (9)
.github/workflows/pipeline_testing.ymlCHANGELOG.mdbrainsets/datasets/PetersonBruntonPoseTrajectory2022.pybrainsets/datasets/__init__.pybrainsets/utils/dandi_utils.pybrainsets/utils/split.pybrainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.pytests/test_dandi_utils.pytests/test_split_utils.py
💤 Files with no reviewable changes (1)
- .github/workflows/pipeline_testing.yml
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- brainsets/utils/dandi_utils.py
estefanysuarez
left a comment
estefanysuarez
left a comment
There was a problem hiding this comment.
I left some minor comments. This PR also made me realize that there are other stuff I have to add in the docs for the three example OpenNeuro brainsets.
| N_FOLDS = 3 | ||
|
|
||
|
|
||
| def _empty_interval() -> Interval: |
There was a problem hiding this comment.
I have seen this function in a couple other brainsets. I wonder if we should add to brainsets/datasets/_utils. It can be useful across datasets.
There was a problem hiding this comment.
Oh? I couldn't find any with the same function. Which brainsets did you have in mind?
| normalized_species = str(raw_species).strip() | ||
| if not normalized_species: | ||
| return Species.UNKNOWN | ||
| if "NCBITaxon" in normalized_species: |
There was a problem hiding this comment.
Maybe add an inline comment to explain what this prefix mean and briefly why this is done?
There was a problem hiding this comment.
The diff is not perfect but this line was actually there before! Honestly I'm not sure what it means but maybe @vinamarora8 will have a better idea
|
|
||
| Full path of the downloaded path will be ``raw_dir / path``. | ||
|
|
||
| The three download modes are evaluated in priority order: |
There was a problem hiding this comment.
I haven't thought this through but if it is feasible and doesn't break things, I think a single arg with multiple values using a Literal would be better?
Something like this:
def download_file(
path: str | Path,
url: str,
raw_dir: str | Path,
download_policy: Literal["skip", "overwrite", "error"],
) -> Path:
...
| "pytest-cov", | ||
| "scikit-learn>=1.6.1", | ||
| "torch_brain@git+https://github.com/neuro-galaxy/torch_brain", | ||
| "torch_brain@git+https://github.com/neuro-galaxy/torch_brain@main", |
There was a problem hiding this comment.
I thought you preferred to control the source branch with
[tool.uv.sources]
torch-brain = { git = "https://github.com/neuro-galaxy/torch_brain", branch = "main" }
…r better control over file existence handling. Update pipeline scripts to reflect this change, replacing overwrite and skip_existing flags with a unified download_policy argument. Adjust dependencies in pyproject.toml for torch_brain.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (4)
497-500: Passstrict=True(orstrict=False) explicitly tozip.Ruff B905 wants the explicit form. Functionally
AJILE_KEYPOINTSandAJILE_NWB_KEYPOINT_NAMESare constants of equal length, sostrict=Trueis the correct intent and would catch a future drift between the two lists immediately.🔧 Proposed fix
keypoint_series = { attr: position[nwb_name] - for attr, nwb_name in zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES) + for attr, nwb_name in zip( + AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, strict=True + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 497 - 500, The dict comprehension building keypoint_series uses zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES) without an explicit strict mode; change the zip call in the keypoint_series comprehension to zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, strict=True) so mismatched lengths raise immediately (or strict=False if you intentionally allow unequal lengths), updating the expression that constructs keypoint_series in pipeline.py accordingly.
313-315: Minor: prefer direct attribute access overgetattr(self.args, "resample_rate").Since
--resample_rateis declared withdefault=500.0,self.args.resample_rateis always defined; thegetattrform (without a default) is equivalent and Ruff flags it (B009). Same situation forgetattr(self.args, "redownload", False)at line 161 andgetattr(self.args, "reprocess", False)at line 301 — those at least pass a default, but if the parser is the single source of truth here, direct access reads cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 313 - 315, Replace the getattr calls on self.args with direct attribute access: use self.args.resample_rate instead of getattr(self.args, "resample_rate"), self.args.redownload instead of getattr(self.args, "redownload", False), and self.args.reprocess instead of getattr(self.args, "reprocess", False); update the code paths where these appear (e.g., the resample_rate assignment and the places that check redownload/reprocess) so they reference the argument attributes directly.
458-461: Preserve original exception when re-raising the scipyImportError.Ruff (B904) flagged this; using
raise ... from err(orfrom Noneif you intentionally suppress chain) makes the missing-dep diagnostic clearer if scipy fails to import for an unusual reason.🔧 Proposed fix
try: from scipy import signal - except ImportError: - raise ImportError("resample_ecog_ajile requires scipy") + except ImportError as err: + raise ImportError("resample_ecog_ajile requires scipy") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 458 - 461, The current import block swallows the original ImportError; update the try/except around "from scipy import signal" to catch the exception as a variable (e.g., "except ImportError as err") and re-raise the new ImportError("resample_ecog_ajile requires scipy") from err so the original traceback is preserved; locate the import of scipy.signal in pipeline.py (the block that raises "resample_ecog_ajile requires scipy") and change the except clause to re-raise using "from err".
408-415: Misleading attribute name:pose_valid_domainactually stores the pose ∩ ECoG joint domain.
signal_valid_domain = pose_valid_domain & ecog_valid_domain(line 358), then it's stored aspose_valid_domain=signal_valid_domain(line 412). Downstream,PetersonBruntonPoseTrajectory2022._get_intersubject_or_intersession_intervalsreturnsdata.pose_valid_domainfortask_type="pose_estimation"(line 188 of the dataset module), so users requesting "pose-valid" intervals actually get the joint signal-valid domain. Functionally fine for training a pose model on top of clean ECoG, but the name will mislead anyone debugging or extending the dataset.Consider renaming to
signal_valid_domain(and updating the dataset class accessor) — or, if the joint domain is intentionally whatpose_valid_domainshould mean for this dataset, document it explicitly in the dataset docstring next to thepose_estimationtask description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 408 - 415, The pipeline is passing the joint pose∩ECoG domain into the keyword pose_valid_domain, which is misleading; change the keyword in the constructor call in pipeline.py from pose_valid_domain=signal_valid_domain to signal_valid_domain=signal_valid_domain (or add a new signal_valid_domain parameter) and then update the dataset accessor/uses in PetersonBruntonPoseTrajectory2022 (e.g., _get_intersubject_or_intersession_intervals and any references to data.pose_valid_domain) to use data.signal_valid_domain (or add a compatibility property and update the class docstring to explain that pose_estimation uses the joint signal_valid_domain if you choose to keep the old name). Ensure the constructor signature and attribute names in PetersonBruntonPoseTrajectory2022 are consistent after the change.brainsets/utils/dandi_utils.py (1)
318-333: Memory: full ECoG load as float64.
np.asarray(electrical_series.data, dtype=np.float64)materializes the entireElectricalSeries(typically an h5py dataset) at double precision before any decimation downstream. For AJILE12 sessions this is multi-GB per session; the docstring already warns, so this is just a heads-up that a chunked path (read-as-native-dtype, decimate per chunk) would let this helper be used on long recordings without blowing the RAM budget. Not a blocker for this PR sinceresample_ecog_ajilealready chunks the resampling step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/dandi_utils.py` around lines 318 - 333, The code eagerly materializes the entire ElectricalSeries into float64 via np.asarray(electrical_series.data, dtype=np.float64), which can OOM for long recordings; change the read path to preserve the dataset's native dtype and perform chunked reads+decimation instead of one full-array cast: read from electrical_series.data using its native dtype (use electrical_series.data.dtype) and iterate over time slices, convert each slice to float64 only when needed for resampling/decimation, append or write chunks to the final storage (or use a memmap/streaming writer) so n_samples/times_out and downstream resample_ecog_ajile can operate without loading the whole dataset into memory. Ensure bad_channels/good logic still reads electrodes["good"][:] as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brainsets/utils/dandi_utils.py`:
- Around line 360-370: The ecog domain end is off-by-one: change construction of
the Interval used for RegularTimeSeries (the variable domain passed to
RegularTimeSeries and the returned ecog_rts) so its end uses n_samples /
sampling_rate rather than times_out[-1]; follow the convention used in
_contiguous_valid_intervals (which uses (ends_idx + 1) / sampling_rate). Update
the Interval(... end=...) expression in brainsets/utils/dandi_utils.py (the
domain variable for ecog_rts) and make the corresponding analogous fixes where
resampled ECoG and pose domains are created in
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py so all
three use the exclusive-end convention consistently.
---
Nitpick comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 497-500: The dict comprehension building keypoint_series uses
zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES) without an explicit strict mode;
change the zip call in the keypoint_series comprehension to zip(AJILE_KEYPOINTS,
AJILE_NWB_KEYPOINT_NAMES, strict=True) so mismatched lengths raise immediately
(or strict=False if you intentionally allow unequal lengths), updating the
expression that constructs keypoint_series in pipeline.py accordingly.
- Around line 313-315: Replace the getattr calls on self.args with direct
attribute access: use self.args.resample_rate instead of getattr(self.args,
"resample_rate"), self.args.redownload instead of getattr(self.args,
"redownload", False), and self.args.reprocess instead of getattr(self.args,
"reprocess", False); update the code paths where these appear (e.g., the
resample_rate assignment and the places that check redownload/reprocess) so they
reference the argument attributes directly.
- Around line 458-461: The current import block swallows the original
ImportError; update the try/except around "from scipy import signal" to catch
the exception as a variable (e.g., "except ImportError as err") and re-raise the
new ImportError("resample_ecog_ajile requires scipy") from err so the original
traceback is preserved; locate the import of scipy.signal in pipeline.py (the
block that raises "resample_ecog_ajile requires scipy") and change the except
clause to re-raise using "from err".
- Around line 408-415: The pipeline is passing the joint pose∩ECoG domain into
the keyword pose_valid_domain, which is misleading; change the keyword in the
constructor call in pipeline.py from pose_valid_domain=signal_valid_domain to
signal_valid_domain=signal_valid_domain (or add a new signal_valid_domain
parameter) and then update the dataset accessor/uses in
PetersonBruntonPoseTrajectory2022 (e.g.,
_get_intersubject_or_intersession_intervals and any references to
data.pose_valid_domain) to use data.signal_valid_domain (or add a compatibility
property and update the class docstring to explain that pose_estimation uses the
joint signal_valid_domain if you choose to keep the old name). Ensure the
constructor signature and attribute names in PetersonBruntonPoseTrajectory2022
are consistent after the change.
In `@brainsets/utils/dandi_utils.py`:
- Around line 318-333: The code eagerly materializes the entire ElectricalSeries
into float64 via np.asarray(electrical_series.data, dtype=np.float64), which can
OOM for long recordings; change the read path to preserve the dataset's native
dtype and perform chunked reads+decimation instead of one full-array cast: read
from electrical_series.data using its native dtype (use
electrical_series.data.dtype) and iterate over time slices, convert each slice
to float64 only when needed for resampling/decimation, append or write chunks to
the final storage (or use a memmap/streaming writer) so n_samples/times_out and
downstream resample_ecog_ajile can operate without loading the whole dataset
into memory. Ensure bad_channels/good logic still reads electrodes["good"][:] as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d62c7ee3-6295-4f5c-af0d-d40b425a3204
📒 Files selected for processing (8)
brainsets/datasets/PetersonBruntonPoseTrajectory2022.pybrainsets/utils/dandi_utils.pybrainsets_pipelines/churchland_shenoy_neural_2012/pipeline.pybrainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.pybrainsets_pipelines/perich_miller_population_2018/pipeline.pybrainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.pypyproject.tomltests/test_dandi_utils.py
✅ Files skipped from review due to trivial changes (1)
- brainsets_pipelines/churchland_shenoy_neural_2012/pipeline.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (3)
294-296:⚠️ Potential issue | 🟡 MinorReplace
assertwith explicitValueErrorfor input validation.
assertis stripped underpython -O, which would silently produce a malformedsession_id(e.g.,AJILE12_P_<date>_…).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 294 - 296, The current input check uses assert on subject_num and subject_num.isdigit(), which can be skipped with python -O; replace it with an explicit runtime check that raises a ValueError when the condition fails. Locate the block where subject_num is computed (the lines referencing subject_num and subject.id in pipeline.py) and change the assert into an if-statement that raises ValueError(f"Could not parse numeric subject from id '{subject.id}'") when subject_num is falsy or not subject_num.isdigit(), preserving the original error message.
204-230:⚠️ Potential issue | 🔴 Critical
Interval.subdivide()still dropsbehavior_labels— stratified folding will crash.
behavior_chunksandavi_chunksare produced viasubdivide()and then handed togenerate_stratified_folds_by_task(..., "behavior_labels", ...). Per prior verification,temporaldata.Interval.subdivide()does not propagate auxiliary attributes, so the stratified-fold call will raise as soon as a session has any active or active/inactive trials. Replicatebehavior_labels(and any other required attrs) onto each sub-interval before calling the fold generator (the same pattern used in_trim_trials_to_domainat lines 588‑600).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 204 - 230, Subdivide() drops auxiliary attrs so generate_stratified_folds_by_task will fail because behavior_chunks and avi_chunks lack behavior_labels; after creating behavior_chunks and avi_chunks (from Interval.subdivide()), copy the session-level/interval-level auxiliary attributes (at least "behavior_labels" and any other attrs used by the task configs) onto each sub-interval before calling generate_stratified_folds_by_task, following the same replication pattern used in _trim_trials_to_domain to propagate attrs onto subdivided intervals; update the blocks that produce behavior_chunks and avi_chunks to perform this attribute replication prior to the stratified-fold calls.
380-384:⚠️ Potential issue | 🟡 MinorGuard against
ZeroDivisionErroron the trim-ratio log.If
original_avi_duration == 0.0(zero-length AVI intervals reaching this branch) thelogger.infocall aborts processing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 380 - 384, The log computes a percentage using original_avi_duration and will raise ZeroDivisionError when original_avi_duration == 0.0; update the logging in pipeline.py (the logger.info call around the trimmed active_vs_inactive message) to compute the percent removed with a guard—e.g., compute percent_removed only if original_avi_duration is nonzero (otherwise set to 0.0 or "N/A") and then include that safe value in the formatted message referencing original_avi_duration and trimmed_avi_duration so the log never divides by zero.
🧹 Nitpick comments (3)
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (3)
500-504: Usezip(..., strict=True)to catch keypoint-list drift (Ruff B905).
AJILE_KEYPOINTSandAJILE_NWB_KEYPOINT_NAMESmust stay length-matched; making it explicit fails fast if someone edits one list:- keypoint_series = { - attr: position[nwb_name] - for attr, nwb_name in zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES) - } + keypoint_series = { + attr: position[nwb_name] + for attr, nwb_name in zip( + AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, strict=True + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 500 - 504, The comprehension building keypoint_series uses zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES) which silently tolerates length mismatches; change it to use zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, strict=True) so the code fails fast if the two lists diverge (update the comprehension around position and keypoint_series accordingly).
318-318: Dropgetattrfor a known attribute.
--resample_ratehas a default of500.0, soself.args.resample_rateis always set;getattrwith no default is just attribute access (Ruff B009).- resample_rate = getattr(self.args, "resample_rate") + resample_rate = self.args.resample_rate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` at line 318, The line using getattr to read the known argument should be replaced with direct attribute access: change the use of getattr(self.args, "resample_rate") to self.args.resample_rate in the pipeline code (reference: resample_rate variable assignment in pipeline.py). Remove the unnecessary getattr call to satisfy Ruff B009 and keep behavior identical since --resample_rate has a default.
461-464: Chain theImportError(Ruff B904).Inside an
exceptclause, re-raising should preserve / explicitly suppress the original cause:- try: - from scipy import signal - except ImportError: - raise ImportError("resample_ecog_ajile requires scipy") + try: + from scipy import signal + except ImportError as err: + raise ImportError("resample_ecog_ajile requires scipy") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 461 - 464, The ImportError raised when scipy isn't available should chain the original exception instead of discarding it; in the try/except around "from scipy import signal" capture the exception (e) in the except and re-raise the new ImportError("resample_ecog_ajile requires scipy") from e so the original cause is preserved (update the import block that guards resample_ecog_ajile).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 466-494: The code computes downsample_factor = int(current_rate /
resample_rate_hz) which truncates non-integer ratios and then incorrectly labels
the output with sampling_rate=resample_rate_hz and computes times_out using
resample_rate_hz; update the function to compute actual_rate = current_rate /
downsample_factor (after verifying downsample_factor >= 1) and use actual_rate
for the RegularTimeSeries sampling_rate and for times_out / domain, or
alternatively raise an error if current_rate / resample_rate_hz is not an
integer; specifically modify the logic around downsample_factor, times_out =
np.arange(n_out) / actual_rate, and the RegularTimeSeries(...) call so
sampling_rate=actual_rate (or add an explicit validation that current_rate %
resample_rate_hz == 0 and raise ValueError if not).
- Around line 638-656: coarse_behaviors.labels may yield byte strings causing
label.split(", ") and lookups in
_canonicalize_behavior_label/LEGACY_BEHAVIOR_LABEL_ALIASES to fail; before
parsing, normalize coarse_behaviors_labels to str by detecting bytes (and
handling empty lists) and decoding each entry (e.g., label.decode("utf-8") for
bytes), and optionally validate/strip None or non-str values so downstream code
that uses parsed_behavior_labels, ACTIVE_BEHAVIOR_PRIORITY and
INACTIVE_BEHAVIORS works with proper Python str objects.
In `@brainsets/datasets/PetersonBruntonPoseTrajectory2022.py`:
- Around line 49-110: Move the large triple-quoted docstring so it is the very
first statement in the class body of PetersonBruntonPoseTrajectory2022 (i.e.,
immediately after the class header), before the class attributes
ACTIVE_BEHAVIOR_LABELS, ACTIVE_BEHAVIOR_TO_ID, INACTIVE_BEHAVIORS,
ACTIVE_VS_INACTIVE_LABELS, and ACTIVE_VS_INACTIVE_TO_ID; this ensures
PetersonBruntonPoseTrajectory2022.__doc__ is populated correctly for help() and
Sphinx autodoc.
---
Duplicate comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 294-296: The current input check uses assert on subject_num and
subject_num.isdigit(), which can be skipped with python -O; replace it with an
explicit runtime check that raises a ValueError when the condition fails. Locate
the block where subject_num is computed (the lines referencing subject_num and
subject.id in pipeline.py) and change the assert into an if-statement that
raises ValueError(f"Could not parse numeric subject from id '{subject.id}'")
when subject_num is falsy or not subject_num.isdigit(), preserving the original
error message.
- Around line 204-230: Subdivide() drops auxiliary attrs so
generate_stratified_folds_by_task will fail because behavior_chunks and
avi_chunks lack behavior_labels; after creating behavior_chunks and avi_chunks
(from Interval.subdivide()), copy the session-level/interval-level auxiliary
attributes (at least "behavior_labels" and any other attrs used by the task
configs) onto each sub-interval before calling
generate_stratified_folds_by_task, following the same replication pattern used
in _trim_trials_to_domain to propagate attrs onto subdivided intervals; update
the blocks that produce behavior_chunks and avi_chunks to perform this attribute
replication prior to the stratified-fold calls.
- Around line 380-384: The log computes a percentage using original_avi_duration
and will raise ZeroDivisionError when original_avi_duration == 0.0; update the
logging in pipeline.py (the logger.info call around the trimmed
active_vs_inactive message) to compute the percent removed with a guard—e.g.,
compute percent_removed only if original_avi_duration is nonzero (otherwise set
to 0.0 or "N/A") and then include that safe value in the formatted message
referencing original_avi_duration and trimmed_avi_duration so the log never
divides by zero.
---
Nitpick comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 500-504: The comprehension building keypoint_series uses
zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES) which silently tolerates length
mismatches; change it to use zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES,
strict=True) so the code fails fast if the two lists diverge (update the
comprehension around position and keypoint_series accordingly).
- Line 318: The line using getattr to read the known argument should be replaced
with direct attribute access: change the use of getattr(self.args,
"resample_rate") to self.args.resample_rate in the pipeline code (reference:
resample_rate variable assignment in pipeline.py). Remove the unnecessary
getattr call to satisfy Ruff B009 and keep behavior identical since
--resample_rate has a default.
- Around line 461-464: The ImportError raised when scipy isn't available should
chain the original exception instead of discarding it; in the try/except around
"from scipy import signal" capture the exception (e) in the except and re-raise
the new ImportError("resample_ecog_ajile requires scipy") from e so the original
cause is preserved (update the import block that guards resample_ecog_ajile).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7004b918-4fd8-455e-a7c8-324c967277fb
📒 Files selected for processing (3)
brainsets/datasets/PetersonBruntonPoseTrajectory2022.pybrainsets/datasets/__init__.pybrainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py
🚧 Files skipped from review as they are similar to previous changes (1)
- brainsets/datasets/init.py
| current_rate = float(ecog_rts.sampling_rate) | ||
| data = np.asarray(ecog_rts.signal[:], dtype=np.float64) | ||
| n_samples, _ = data.shape | ||
| downsample_factor = int(current_rate / resample_rate_hz) | ||
| if downsample_factor < 1: | ||
| raise ValueError("resample_rate_hz must be <= native rate for decimation") | ||
| chunk_samples = int(chunk_duration_sec * current_rate) | ||
|
|
||
| downsampled_chunks = [] | ||
| for start_idx in tqdm( | ||
| range(0, n_samples, chunk_samples), desc="Resampling ECoG chunks" | ||
| ): | ||
| end_idx = min(start_idx + chunk_samples, n_samples) | ||
| chunk = data[start_idx:end_idx, :] | ||
| if downsample_factor > 1: | ||
| chunk = signal.decimate( | ||
| chunk, downsample_factor, axis=0, ftype="iir", zero_phase=True | ||
| ) | ||
| downsampled_chunks.append(chunk) | ||
|
|
||
| data_out = np.concatenate(downsampled_chunks, axis=0) | ||
| n_out = data_out.shape[0] | ||
| times_out = np.arange(n_out) / resample_rate_hz | ||
| domain = Interval(start=np.array([times_out[0]]), end=np.array([times_out[-1]])) | ||
| return RegularTimeSeries( | ||
| signal=data_out, | ||
| sampling_rate=resample_rate_hz, | ||
| domain=domain, | ||
| ) |
There was a problem hiding this comment.
Reported sampling_rate may be wrong when current_rate is not an integer multiple of resample_rate_hz.
downsample_factor = int(current_rate / resample_rate_hz) truncates, but the returned RegularTimeSeries is labelled sampling_rate=resample_rate_hz. The true output rate is current_rate / downsample_factor. E.g. native 1024 Hz with --resample_rate 500 produces 512 Hz data tagged as 500 Hz, which silently misaligns timestamps for downstream consumers (and times_out = np.arange(n_out)/resample_rate_hz is similarly off).
For AJILE12's typical 500 Hz native rate the default config dodges this, but any non-integer ratio passed via --resample_rate (e.g. 200 Hz) trips it. Either (a) compute actual_rate = current_rate / downsample_factor and use that for the returned sampling_rate/domain, or (b) reject non-integer ratios up front.
♻️ Proposed fix
current_rate = float(ecog_rts.sampling_rate)
data = np.asarray(ecog_rts.signal[:], dtype=np.float64)
n_samples, _ = data.shape
- downsample_factor = int(current_rate / resample_rate_hz)
- if downsample_factor < 1:
- raise ValueError("resample_rate_hz must be <= native rate for decimation")
+ ratio = current_rate / resample_rate_hz
+ downsample_factor = int(round(ratio))
+ if downsample_factor < 1:
+ raise ValueError("resample_rate_hz must be <= native rate for decimation")
+ if not np.isclose(ratio, downsample_factor):
+ raise ValueError(
+ f"native rate {current_rate} Hz is not an integer multiple of "
+ f"resample_rate_hz {resample_rate_hz} Hz"
+ )
@@
- n_out = data_out.shape[0]
- times_out = np.arange(n_out) / resample_rate_hz
- domain = Interval(start=np.array([times_out[0]]), end=np.array([times_out[-1]]))
+ actual_rate = current_rate / downsample_factor
+ n_out = data_out.shape[0]
+ times_out = np.arange(n_out) / actual_rate
+ domain = Interval(start=np.array([times_out[0]]), end=np.array([times_out[-1]]))
return RegularTimeSeries(
signal=data_out,
- sampling_rate=resample_rate_hz,
+ sampling_rate=actual_rate,
domain=domain,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| current_rate = float(ecog_rts.sampling_rate) | |
| data = np.asarray(ecog_rts.signal[:], dtype=np.float64) | |
| n_samples, _ = data.shape | |
| downsample_factor = int(current_rate / resample_rate_hz) | |
| if downsample_factor < 1: | |
| raise ValueError("resample_rate_hz must be <= native rate for decimation") | |
| chunk_samples = int(chunk_duration_sec * current_rate) | |
| downsampled_chunks = [] | |
| for start_idx in tqdm( | |
| range(0, n_samples, chunk_samples), desc="Resampling ECoG chunks" | |
| ): | |
| end_idx = min(start_idx + chunk_samples, n_samples) | |
| chunk = data[start_idx:end_idx, :] | |
| if downsample_factor > 1: | |
| chunk = signal.decimate( | |
| chunk, downsample_factor, axis=0, ftype="iir", zero_phase=True | |
| ) | |
| downsampled_chunks.append(chunk) | |
| data_out = np.concatenate(downsampled_chunks, axis=0) | |
| n_out = data_out.shape[0] | |
| times_out = np.arange(n_out) / resample_rate_hz | |
| domain = Interval(start=np.array([times_out[0]]), end=np.array([times_out[-1]])) | |
| return RegularTimeSeries( | |
| signal=data_out, | |
| sampling_rate=resample_rate_hz, | |
| domain=domain, | |
| ) | |
| current_rate = float(ecog_rts.sampling_rate) | |
| data = np.asarray(ecog_rts.signal[:], dtype=np.float64) | |
| n_samples, _ = data.shape | |
| ratio = current_rate / resample_rate_hz | |
| downsample_factor = int(round(ratio)) | |
| if downsample_factor < 1: | |
| raise ValueError("resample_rate_hz must be <= native rate for decimation") | |
| if not np.isclose(ratio, downsample_factor): | |
| raise ValueError( | |
| f"native rate {current_rate} Hz is not an integer multiple of " | |
| f"resample_rate_hz {resample_rate_hz} Hz" | |
| ) | |
| chunk_samples = int(chunk_duration_sec * current_rate) | |
| downsampled_chunks = [] | |
| for start_idx in tqdm( | |
| range(0, n_samples, chunk_samples), desc="Resampling ECoG chunks" | |
| ): | |
| end_idx = min(start_idx + chunk_samples, n_samples) | |
| chunk = data[start_idx:end_idx, :] | |
| if downsample_factor > 1: | |
| chunk = signal.decimate( | |
| chunk, downsample_factor, axis=0, ftype="iir", zero_phase=True | |
| ) | |
| downsampled_chunks.append(chunk) | |
| data_out = np.concatenate(downsampled_chunks, axis=0) | |
| actual_rate = current_rate / downsample_factor | |
| n_out = data_out.shape[0] | |
| times_out = np.arange(n_out) / actual_rate | |
| domain = Interval(start=np.array([times_out[0]]), end=np.array([times_out[-1]])) | |
| return RegularTimeSeries( | |
| signal=data_out, | |
| sampling_rate=actual_rate, | |
| domain=domain, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around
lines 466 - 494, The code computes downsample_factor = int(current_rate /
resample_rate_hz) which truncates non-integer ratios and then incorrectly labels
the output with sampling_rate=resample_rate_hz and computes times_out using
resample_rate_hz; update the function to compute actual_rate = current_rate /
downsample_factor (after verifying downsample_factor >= 1) and use actual_rate
for the RegularTimeSeries sampling_rate and for times_out / domain, or
alternatively raise an error if current_rate / resample_rate_hz is not an
integer; specifically modify the logic around downsample_factor, times_out =
np.arange(n_out) / actual_rate, and the RegularTimeSeries(...) call so
sampling_rate=actual_rate (or add an explicit validation that current_rate %
resample_rate_hz == 0 and raise ValueError if not).
| coarse_behaviors = nwbfile.intervals["epochs"] | ||
| coarse_behaviors_labels = coarse_behaviors.labels.data[:].tolist() | ||
| start_times = np.round(coarse_behaviors.start_time.data[:], 3) | ||
| end_times = np.round(coarse_behaviors.stop_time.data[:], 3) | ||
|
|
||
| active_event_set = set(ACTIVE_BEHAVIOR_PRIORITY) | ||
| inactive_event_set = INACTIVE_BEHAVIORS | ||
|
|
||
| parsed_behavior_labels = [ | ||
| [_canonicalize_behavior_label(event) for event in label.split(", ")] | ||
| for label in coarse_behaviors_labels | ||
| ] | ||
|
|
||
| active_event_mask = np.array( | ||
| [all(event in active_event_set for event in events) for events in parsed_behavior_labels] | ||
| ) | ||
| inactive_event_mask = np.array( | ||
| [all(event in inactive_event_set for event in events) for events in parsed_behavior_labels] | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# See how other AJILE/NWB pipelines in this repo treat label arrays from epochs
rg -nP --type=py -C3 '\.labels\.data\b|epochs\["labels"\]|labels\.data\[' 2>/dev/nullRepository: neuro-galaxy/brainsets
Length of output: 849
🏁 Script executed:
#!/bin/bash
# Find and examine _canonicalize_behavior_label function
rg -n "_canonicalize_behavior_label" --type=py -B2 -A10Repository: neuro-galaxy/brainsets
Length of output: 2856
🏁 Script executed:
#!/bin/bash
# Find LEGACY_BEHAVIOR_LABEL_ALIASES and ACTIVE_BEHAVIOR_PRIORITY definitions
rg -n "LEGACY_BEHAVIOR_LABEL_ALIASES|ACTIVE_BEHAVIOR_PRIORITY" --type=py -B1 -A3Repository: neuro-galaxy/brainsets
Length of output: 3336
🏁 Script executed:
#!/bin/bash
# Get context around lines 638-656 in the pipeline file
sed -n '620,670p' brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py | cat -nRepository: neuro-galaxy/brainsets
Length of output: 2410
🏁 Script executed:
#!/bin/bash
# Check if there's any type validation or decoding elsewhere in codebase for NWB label data
rg -n "decode|bytes|str\(" brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py | head -20Repository: neuro-galaxy/brainsets
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Look at the actual data structure to understand what h5py/NWB returns
rg -n "nwbfile\.intervals|\.labels\.data" brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py -B2 -A2Repository: neuro-galaxy/brainsets
Length of output: 562
🏁 Script executed:
#!/bin/bash
# Check if there are tests for this pipeline with different NWB versions
fd -e py test | xargs rg -l "peterson_brunton|PetersonBrunton" 2>/dev/null | head -5Repository: neuro-galaxy/brainsets
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check what PyNWB/h5py typically returns for string datasets
# Also look for any encoding specifications in NWB schema or comments
rg -n "encoding|h5py|HDF5|bytes" brainsets_pipelines/ --type=py | head -20Repository: neuro-galaxy/brainsets
Length of output: 2100
🏁 Script executed:
#!/bin/bash
# Look at how ACTIVE_BEHAVIOR_LABELS is defined - gives us clue about str format
rg -n "ACTIVE_BEHAVIOR_LABELS" brainsets/ --type=py -B1 -A5Repository: neuro-galaxy/brainsets
Length of output: 2996
🏁 Script executed:
#!/bin/bash
# Check if there are any actual test files or data samples
fd -name "*.nwb" | head -5Repository: neuro-galaxy/brainsets
Length of output: 297
🏁 Script executed:
#!/bin/bash
# Look for any NWB IO or data reading code that might show encoding handling
rg -n "NWBHDF5IO|h5py\.File|\.labels" brainsets_pipelines/peterson_brunton_pose_trajectory_2022/ --type=py -C2Repository: neuro-galaxy/brainsets
Length of output: 2395
🏁 Script executed:
#!/bin/bash
# Check PyNWB version requirements to see if there's guaranteed str decoding
rg -n "pynwb|requirements" brainsets_pipelines/peterson_brunton_pose_trajectory_2022/ --type=py -B1 -A1 | head -20Repository: neuro-galaxy/brainsets
Length of output: 688
🏁 Script executed:
#!/bin/bash
# Look at the full context of where _canonicalize_behavior_label is called
sed -n '640,660p' brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py | cat -nRepository: neuro-galaxy/brainsets
Length of output: 1085
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about label encoding in the NWB structure
rg -n "labels|encoding|utf" brainsets/datasets/PetersonBruntonPoseTrajectory2022.py -B1 -A1Repository: neuro-galaxy/brainsets
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Look for any setup.py or pyproject.toml to see PyNWB version constraints
find . -name "pyproject.toml" -o -name "setup.py" -o -name "setup.cfg" -o -name "requirements*.txt" | xargs cat 2>/dev/null | grep -i pynwbRepository: neuro-galaxy/brainsets
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Look at PyNWB changelog/docs for string handling in version 2.2+
# Also check if there's any actual usage of this code with real NWB files
rg -n "\.split\(|bytes|decode|str\(" brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py | grep -A2 -B2 "647\|decode"Repository: neuro-galaxy/brainsets
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Check if _canonicalize_behavior_label has type checking or try/except
sed -n '111,120p' brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py | cat -nRepository: neuro-galaxy/brainsets
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Look at the full block to see error handling
sed -n '645,675p' brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py | cat -nRepository: neuro-galaxy/brainsets
Length of output: 1556
Add defensive decoding for NWB label strings.
Line 639 (coarse_behaviors.labels.data[:].tolist()) may return bytes if the HDF5 dataset uses fixed-length byte strings (common in older NWB files). This causes line 647 to fail with TypeError when calling label.split(", ") on bytes with a str separator. Additionally, dict lookups in _canonicalize_behavior_label against LEGACY_BEHAVIOR_LABEL_ALIASES (str keys) require str input. Explicitly decode labels or add type validation:
coarse_behaviors_labels = coarse_behaviors.labels.data[:].tolist()
# Decode if bytes
if coarse_behaviors_labels and isinstance(coarse_behaviors_labels[0], bytes):
coarse_behaviors_labels = [label.decode('utf-8') for label in coarse_behaviors_labels]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around
lines 638 - 656, coarse_behaviors.labels may yield byte strings causing
label.split(", ") and lookups in
_canonicalize_behavior_label/LEGACY_BEHAVIOR_LABEL_ALIASES to fail; before
parsing, normalize coarse_behaviors_labels to str by detecting bytes (and
handling empty lists) and decoding each entry (e.g., label.decode("utf-8") for
bytes), and optionally validate/strip None or non-str values so downstream code
that uses parsed_behavior_labels, ACTIVE_BEHAVIOR_PRIORITY and
INACTIVE_BEHAVIORS works with proper Python str objects.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (5)
318-320:⚠️ Potential issue | 🟠 MajorReplace
assertwith an explicitValueErrorfor input validation.Asserts are stripped under
python -O, which would silently let a malformedsubject_numpropagate intosession_id(e.g.AJILE12_P_<date>_…). Use a real exception so this fails deterministically regardless of optimization flags.Proposed fix
- assert ( - subject_num and subject_num.isdigit() - ), f"Could not parse numeric subject from id '{subject.id}'" + if not (subject_num and subject_num.isdigit()): + raise ValueError( + f"Could not parse numeric subject from id '{subject.id}'" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 318 - 320, Replace the runtime-only assert with explicit input validation: check that subject_num is truthy and subject_num.isdigit(), and if not raise a ValueError with the message "Could not parse numeric subject from id '<subject.id>'" (referencing subject.id and subject_num in the validation) so the code that constructs session_id fails deterministically even under python -O.
414-418:⚠️ Potential issue | 🟡 MinorGuard the trim-ratio log against
original_avi_duration == 0.If
active_vs_inactive_trialsis non-empty but consists of degenerate zero-duration intervals,original_avi_durationis0.0and thef"…{100 * (orig - trimmed) / orig:.1f}%"interpolation raisesZeroDivisionError, aborting processing of the session.Proposed fix
- logger.info( - f"Trimmed active_vs_inactive trials to valid signal domain: " - f"{original_avi_duration:.1f}s -> {trimmed_avi_duration:.1f}s " - f"({100 * (original_avi_duration - trimmed_avi_duration) / original_avi_duration:.1f}% removed)." - ) + pct_removed = ( + 100 * (original_avi_duration - trimmed_avi_duration) / original_avi_duration + if original_avi_duration > 0 + else 0.0 + ) + logger.info( + f"Trimmed active_vs_inactive trials to valid signal domain: " + f"{original_avi_duration:.1f}s -> {trimmed_avi_duration:.1f}s " + f"({pct_removed:.1f}% removed)." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 414 - 418, The log interpolation can divide by zero when original_avi_duration == 0; guard the calculation in the logger.info call by checking original_avi_duration first (and reference the active_vs_inactive_trials context if helpful) and compute a safe percent_removed value (e.g., 0.0 or "N/A") when original_avi_duration is zero, then log using original_avi_duration, trimmed_avi_duration and the safe percent_removed; update the logger.info invocation that uses original_avi_duration and trimmed_avi_duration to use this guarded percent calculation.
795-806:⚠️ Potential issue | 🟡 MinorDecode label bytes before string operations.
coarse_behaviors.labels.data[:]may returnbytesfor older NWB files written with fixed-length HDF5 byte strings — in which caselabel.split(", ")on line 804 raisesTypeError: a bytes-like object is required, not 'str', and even if it didn't, lookups againstLEGACY_BEHAVIOR_LABEL_ALIASESand the active/inactive sets (all keyed bystr) would silently miss. Normalize once, right after reading:Proposed fix
- coarse_behaviors_labels = coarse_behaviors.labels.data[:].tolist() + coarse_behaviors_labels = coarse_behaviors.labels.data[:].tolist() + coarse_behaviors_labels = [ + label.decode("utf-8") if isinstance(label, bytes) else label + for label in coarse_behaviors_labels + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 795 - 806, coarse_behaviors.labels.data[:] may be bytes; decode those entries to str immediately after reading so downstream string ops and lookups work: after constructing coarse_behaviors_labels (from coarse_behaviors.labels.data[:]) convert any bytes to a decoded string (e.g., utf-8 with errors='replace') before building parsed_behavior_labels, so _canonicalize_behavior_label, LEGACY_BEHAVIOR_LABEL_ALIASES, ACTIVE_BEHAVIOR_PRIORITY and INACTIVE_BEHAVIORS comparisons operate on str values rather than bytes.
228-254:⚠️ Potential issue | 🔴 CriticalStratified-fold call still loses required
behavior_labelsaftersubdivide().
Interval.subdivide()in temporaldata only carriesstart/end, sobehavior_chunksandavi_chunksno longer exposebehavior_labels(orbehavior_id/timestamps) when handed togenerate_stratified_folds_by_task(..., label_field="behavior_labels"). That utility raises when the field is missing, and this branch only runs whenlen(...) > 0, i.e. exactly when it would normally execute.The same auxiliary-attribute replication pattern already used in
_trim_trials_to_domain(lines 749–755) needs to be applied to the chunks before stratified folding — for each subdivided interval, look up the source trial and copybehavior_labels(andbehavior_id/timestamps) onto the new sub-intervals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 228 - 254, The subdivided intervals lose auxiliary attributes (`behavior_labels`, `behavior_id`, `timestamps`) because Interval.subdivide() only preserves start/end; after creating behavior_chunks and avi_chunks (the results of calling active_behavior_trials.subdivide(...) and active_vs_inactive_trials.subdivide(...)), iterate each new chunk and look up its originating source trial (same approach used in _trim_trials_to_domain) and copy the original trial's `behavior_labels`, `behavior_id`, and `timestamps` onto the chunk objects before calling generate_stratified_folds_by_task(..., label_field="behavior_labels"); apply the same replication logic for both `behavior_chunks` and `avi_chunks` so generate_stratified_folds_by_task receives intervals with the required fields.
500-528:⚠️ Potential issue | 🟠 MajorReported
sampling_rateis wrong whencurrent_rateisn't an integer multiple ofresample_rate_hz.
int(current_rate / resample_rate_hz)truncates the ratio, but the returnedRegularTimeSeriesis still taggedsampling_rate=resample_rate_hzandtimes_outis built fromresample_rate_hz. For a 1024 Hz native rate with--resample_rate 200, you get 341 Hz data labelled as 200 Hz (and a domain end that's off by ~40%), silently misaligning every downstream timestamp. AJILE12's nominal 500 Hz with the default 500 Hz config dodges this, but any non-integer ratio passed via--resample_ratetrips it.Either compute
actual_rate = current_rate / downsample_factorand use that for bothsampling_rateandtimes_out, or reject non-integer ratios up front.Proposed fix
current_rate = float(ecog_rts.sampling_rate) data = np.asarray(ecog_rts.signal[:], dtype=np.float64) n_samples, _ = data.shape - downsample_factor = int(current_rate / resample_rate_hz) - if downsample_factor < 1: - raise ValueError("resample_rate_hz must be <= native rate for decimation") + ratio = current_rate / resample_rate_hz + downsample_factor = int(round(ratio)) + if downsample_factor < 1: + raise ValueError("resample_rate_hz must be <= native rate for decimation") + if not np.isclose(ratio, downsample_factor): + raise ValueError( + f"native rate {current_rate} Hz is not an integer multiple of " + f"resample_rate_hz {resample_rate_hz} Hz" + ) @@ data_out = np.concatenate(downsampled_chunks, axis=0) + actual_rate = current_rate / downsample_factor n_out = data_out.shape[0] - times_out = np.arange(n_out) / resample_rate_hz + times_out = np.arange(n_out) / actual_rate domain = Interval(start=np.array([times_out[0]]), end=np.array([times_out[-1]])) return RegularTimeSeries( signal=data_out, - sampling_rate=resample_rate_hz, + sampling_rate=actual_rate, domain=domain, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 500 - 528, The code computes downsample_factor = int(current_rate / resample_rate_hz) which truncates non-integer ratios, so fix by computing actual_rate = current_rate / downsample_factor after determining downsample_factor (and validate downsample_factor >= 1), use actual_rate instead of resample_rate_hz when building times_out, the Interval domain start/end, and when passing sampling_rate into RegularTimeSeries (all referenced symbols: current_rate, downsample_factor, actual_rate, times_out, Interval, RegularTimeSeries, signal.decimate); alternatively, explicitly raise an error if current_rate / resample_rate_hz is not an integer to reject non-integer ratios.
🧹 Nitpick comments (2)
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py (2)
495-498: Chain theImportErrorto preserve the original cause (Ruff B904).Within the
except, useraise … from err(orfrom None) so traceback chaining is explicit.Proposed fix
- try: - from scipy import signal - except ImportError: - raise ImportError("resample_ecog_ajile requires scipy") + try: + from scipy import signal + except ImportError as err: + raise ImportError("resample_ecog_ajile requires scipy") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 495 - 498, The ImportError handler for importing scipy in the resample_ecog_ajile import block should preserve the original exception chain; change the except clause to capture the original exception (e.g., except ImportError as err) and re-raise the new ImportError("resample_ecog_ajile requires scipy") from err so the traceback shows the underlying cause while keeping the existing message.
535-538: Addstrict=Truetozip()call (Ruff B905).
AJILE_KEYPOINTSandAJILE_NWB_KEYPOINT_NAMESmust match in length. Usingstrict=Truewill raise aValueErrorif they differ instead of silently truncating, catching potential bugs earlier. Python 3.12 supports this parameter (available since Python 3.10).Suggested fix
- keypoint_series = { - attr: position[nwb_name] - for attr, nwb_name in zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES) - } + keypoint_series = { + attr: position[nwb_name] + for attr, nwb_name in zip( + AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, strict=True + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around lines 535 - 538, The dict comprehension building keypoint_series silently truncates if AJILE_KEYPOINTS and AJILE_NWB_KEYPOINT_NAMES differ; update the zip call in the comprehension that constructs keypoint_series to use zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, strict=True) so a ValueError is raised on length mismatch (refer to AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, and the keypoint_series expression in pipeline.py).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 643-656: The median calculation can return NaN for an all-NaN
shoulder trace and the current branch logs a misleading "zero" message; change
the computation to use np.nanmedian for ref_distances and then test with
np.isfinite(D) (or np.isfinite on ref_distances result) instead of D > 0, and
only perform the scaling of kp_xy for keys in AJILE_KEYPOINTS when D is a finite
positive number; otherwise emit the warning via logger.warning about an
undefined or non-finite reference distance and skip scale normalization.
---
Duplicate comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 318-320: Replace the runtime-only assert with explicit input
validation: check that subject_num is truthy and subject_num.isdigit(), and if
not raise a ValueError with the message "Could not parse numeric subject from id
'<subject.id>'" (referencing subject.id and subject_num in the validation) so
the code that constructs session_id fails deterministically even under python
-O.
- Around line 414-418: The log interpolation can divide by zero when
original_avi_duration == 0; guard the calculation in the logger.info call by
checking original_avi_duration first (and reference the
active_vs_inactive_trials context if helpful) and compute a safe percent_removed
value (e.g., 0.0 or "N/A") when original_avi_duration is zero, then log using
original_avi_duration, trimmed_avi_duration and the safe percent_removed; update
the logger.info invocation that uses original_avi_duration and
trimmed_avi_duration to use this guarded percent calculation.
- Around line 795-806: coarse_behaviors.labels.data[:] may be bytes; decode
those entries to str immediately after reading so downstream string ops and
lookups work: after constructing coarse_behaviors_labels (from
coarse_behaviors.labels.data[:]) convert any bytes to a decoded string (e.g.,
utf-8 with errors='replace') before building parsed_behavior_labels, so
_canonicalize_behavior_label, LEGACY_BEHAVIOR_LABEL_ALIASES,
ACTIVE_BEHAVIOR_PRIORITY and INACTIVE_BEHAVIORS comparisons operate on str
values rather than bytes.
- Around line 228-254: The subdivided intervals lose auxiliary attributes
(`behavior_labels`, `behavior_id`, `timestamps`) because Interval.subdivide()
only preserves start/end; after creating behavior_chunks and avi_chunks (the
results of calling active_behavior_trials.subdivide(...) and
active_vs_inactive_trials.subdivide(...)), iterate each new chunk and look up
its originating source trial (same approach used in _trim_trials_to_domain) and
copy the original trial's `behavior_labels`, `behavior_id`, and `timestamps`
onto the chunk objects before calling generate_stratified_folds_by_task(...,
label_field="behavior_labels"); apply the same replication logic for both
`behavior_chunks` and `avi_chunks` so generate_stratified_folds_by_task receives
intervals with the required fields.
- Around line 500-528: The code computes downsample_factor = int(current_rate /
resample_rate_hz) which truncates non-integer ratios, so fix by computing
actual_rate = current_rate / downsample_factor after determining
downsample_factor (and validate downsample_factor >= 1), use actual_rate instead
of resample_rate_hz when building times_out, the Interval domain start/end, and
when passing sampling_rate into RegularTimeSeries (all referenced symbols:
current_rate, downsample_factor, actual_rate, times_out, Interval,
RegularTimeSeries, signal.decimate); alternatively, explicitly raise an error if
current_rate / resample_rate_hz is not an integer to reject non-integer ratios.
---
Nitpick comments:
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py`:
- Around line 495-498: The ImportError handler for importing scipy in the
resample_ecog_ajile import block should preserve the original exception chain;
change the except clause to capture the original exception (e.g., except
ImportError as err) and re-raise the new ImportError("resample_ecog_ajile
requires scipy") from err so the traceback shows the underlying cause while
keeping the existing message.
- Around line 535-538: The dict comprehension building keypoint_series silently
truncates if AJILE_KEYPOINTS and AJILE_NWB_KEYPOINT_NAMES differ; update the zip
call in the comprehension that constructs keypoint_series to use
zip(AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES, strict=True) so a ValueError is
raised on length mismatch (refer to AJILE_KEYPOINTS, AJILE_NWB_KEYPOINT_NAMES,
and the keypoint_series expression in pipeline.py).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7caf2100-8c33-4135-acb5-cf57cc11e02e
📒 Files selected for processing (5)
brainsets/ajile_behavior_labels.pybrainsets/datasets/PetersonBruntonPoseTrajectory2022.pybrainsets/datasets/__init__.pybrainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.pypyproject.toml
✅ Files skipped from review due to trivial changes (3)
- pyproject.toml
- brainsets/ajile_behavior_labels.py
- brainsets/datasets/PetersonBruntonPoseTrajectory2022.py
🚧 Files skipped from review as they are similar to previous changes (1)
- brainsets/datasets/init.py
| shoulder_dist = np.linalg.norm( | ||
| kp_xy["l_shoulder"] - kp_xy["r_shoulder"], axis=1 | ||
| ) | ||
| ref_distances = shoulder_dist[high_conf] if high_conf.any() else shoulder_dist | ||
| D = float(np.median(ref_distances)) | ||
|
|
||
| if D > 0: | ||
| for kp in AJILE_KEYPOINTS: | ||
| kp_xy[kp] /= D | ||
| else: | ||
| logger.warning( | ||
| "Shoulder-to-shoulder reference distance is zero; " | ||
| "skipping scale normalization." | ||
| ) |
There was a problem hiding this comment.
np.median will return NaN for an all-NaN trace and the warning will be misleading.
If both shoulder traces were entirely below confidence_threshold and interpolation couldn't fill anything (e.g., valid.sum() < 2 for a column at line 607), kp_xy["l_shoulder"] - kp_xy["r_shoulder"] produces NaNs, D = NaN, D > 0 is False, and the log message claims the distance is "zero" when it's actually undefined. Switching to np.nanmedian and adding a finite check makes the failure mode honest:
Proposed fix
- ref_distances = shoulder_dist[high_conf] if high_conf.any() else shoulder_dist
- D = float(np.median(ref_distances))
-
- if D > 0:
+ ref_distances = shoulder_dist[high_conf] if high_conf.any() else shoulder_dist
+ D = float(np.nanmedian(ref_distances)) if ref_distances.size else 0.0
+
+ if np.isfinite(D) and D > 0:
for kp in AJILE_KEYPOINTS:
kp_xy[kp] /= D
else:
logger.warning(
- "Shoulder-to-shoulder reference distance is zero; "
- "skipping scale normalization."
+ "Shoulder-to-shoulder reference distance is zero or undefined; "
+ "skipping scale normalization."
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.py` around
lines 643 - 656, The median calculation can return NaN for an all-NaN shoulder
trace and the current branch logs a misleading "zero" message; change the
computation to use np.nanmedian for ref_distances and then test with
np.isfinite(D) (or np.isfinite on ref_distances result) instead of D > 0, and
only perform the scaling of kp_xy for keys in AJILE_KEYPOINTS when D is a finite
positive number; otherwise emit the warning via logger.warning about an
undefined or non-finite reference distance and skip scale normalization.
… for file handling. Update pipeline scripts to align with new parameterization for file download behavior.
Replace brainsets.taxonomy enums with plain strings and legacy hemisphere integer codes in dandi_utils and the AJILE pipeline, and align ECoG extraction with the current temporaldata RegularTimeSeries API. Co-authored-by: Cursor <cursoragent@cursor.com>
… representations. Updated the subject extraction functions to directly use the NWB file's subject metadata, ensuring compliance with DANDI's requirements for species and sex. Adjusted related tests to reflect these changes.
2 participants
This PR adds support for AJILE12, a large naturalistic human ECoG + pose dataset introduced in Peterson et al. (Scientific Data, 2022):
https://doi.org/10.1038/s41597-022-01280-y
Summary
brainsets_pipelines/peterson_brunton_pose_trajectory_2022/pipeline.pyto ingest NWB files from DANDI (000055/0.220127.0436) and produce standardized HDF5 recordings.PetersonBruntonPoseTrajectory2022dataset class with task-aware sampling intervals for:active_vs_inactivebehaviorpose_estimationand support for
intrasession,intersession, andintersubjectsplits.RegularTimeSeries) + channel metadataactive_behavior_trials,active_vs_inactive_trials)participants.json) for AJILE12 subject attributes (age/sex/hemisphere) and export the dataset inbrainsets.datasets.brainsets/utils/dandi_utils.pybrainsets/utils/split.pyskip_existingsupport).IMPORTANT NOTE: I used the
peterson_brunton_pose_trajectory_2022name to follow the brainsets convention, but I don't hate the idea of changing that toajile12_2022(following what was done recently with neuroprobe) as it makes it much more digestible and recognizable. Let me know what you think!Dataset citation
Peterson SM, Singh SH, Dichter B, et al. AJILE12: Long-term naturalistic human intracranial neural recordings and pose. Scientific Data. 2022;9:184. https://doi.org/10.1038/s41597-022-01280-y
Summary by CodeRabbit
New Features
Utilities
Documentation
Tests
Chores