Adds RTT task in the pei_pandarinath_nlb_2021 (NLB) brainset - #108
Open
divyansha1115 wants to merge 14 commits into
Open
Adds RTT task in the pei_pandarinath_nlb_2021 (NLB) brainset#108divyansha1115 wants to merge 14 commits into
divyansha1115 wants to merge 14 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPipeline now emits per-task manifests, returns composite download outputs consumed by process, and dispatches task-specific behavior extraction (maze vs RTT). Trial interval boundaries are conditionally rounded to one decimal place. ChangesPipeline core & manifest
Download / Processing
Behavior extraction
Trials
Sequence DiagramsequenceDiagram
participant Client
participant Pipeline
participant Manifest
participant Downloader
participant NWBFile
participant Extractor
Client->>Pipeline: get_manifest(raw_dir)
Pipeline->>Manifest: build per-task manifest entries
Manifest-->>Pipeline: merged DataFrame with task & dandiset_id
Pipeline-->>Client: manifest
Client->>Pipeline: download(manifest_item)
Pipeline->>Downloader: fetch file via dandiset_id & task
Downloader-->>Pipeline: fpath
Pipeline-->>Client: {fpath, manifest_item}
Client->>Pipeline: process(download_output)
Pipeline->>NWBFile: load from fpath
NWBFile-->>Pipeline: nwbfile object
alt task == 'maze'
Pipeline->>Extractor: extract_behavior_maze(nwbfile, trials)
Extractor-->>Pipeline: hand/eye data
else task == 'RTT'
Pipeline->>Extractor: extract_behavior_rtt(nwbfile, trials)
Extractor-->>Pipeline: cursor/finger/target data
end
Pipeline->>Pipeline: create BrainsetDescription
Pipeline-->>Client: brainset data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. ✨ Finishing Touches📝 Generate docstrings
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
divyansha1115
changed the title
divyansha1115
marked this pull request as ready for review
March 19, 2026 02:10
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py (1)
201-214:⚠️ Potential issue | 🔴 CriticalGuard split-mask extraction for RTT files.
extract_trials()is called for every asset at Line 140, but RTT assets are added without predefined splits. Accessingtrial_table.split_indicatorunconditionally will fail before the RTT-specific branch can run.🛡️ Proposed fix
- train_mask_nwb = trial_table.split_indicator.to_numpy() == "train" - test_mask_nwb = trial_table.split_indicator.to_numpy() == "val" - - trials.train_mask_nwb = ( - train_mask_nwb # Naming with "_" since train_mask is reserved - ) - trials.test_mask_nwb = test_mask_nwb # Naming with "_" since test_mask is reserved + if "split_indicator" in trial_table.columns: + train_mask_nwb = trial_table["split_indicator"].to_numpy() == "train" + test_mask_nwb = trial_table["split_indicator"].to_numpy() == "val" + + trials.train_mask_nwb = ( + train_mask_nwb # Naming with "_" since train_mask is reserved + ) + trials.test_mask_nwb = ( + test_mask_nwb # Naming with "_" since test_mask is reserved + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 201 - 214, extract_trials() currently assumes trial_table has a split_indicator column and unconditionally reads trial_table.split_indicator, which breaks for RTT assets that lack predefined splits; to fix, guard the split extraction by checking for the presence of the split_indicator column or attribute on trial_table before using it (e.g., if "split_indicator" in trial_table.columns or hasattr(trial_table, "split_indicator")) and only build train_mask_nwb/test_mask_nwb when present, otherwise initialize trials.train_mask_nwb and trials.test_mask_nwb to safe defaults (e.g., all False or appropriate empty masks) so the RTT-specific branch can run without error; update the code around Interval.from_dataframe(trial_table) and the assignments to trials.train_mask_nwb and trials.test_mask_nwb accordingly.
🤖 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/pei_pandarinath_nlb_2021/pipeline.py`:
- Around line 80-89: The current BrainsetDescription assigned to
brainset_description only mentions the maze center-out reaching task; update the
description field in the BrainsetDescription instantiation
(brainset_description) to reflect that the brainset also contains RTT exports —
broaden the provenance text to reference both the maze/center-out reaching task
and RTT recordings, and mention included data modalities (e.g., sorted unit
spiking times, behavioral measurements, and RTT data) so exported metadata
accurately describes all contents.
- Around line 56-60: The manifest loop currently overwrites m["task"] with only
the suffix (task.split("_")[1]) so downstream process() and the task dispatch
logic (which expect full names like "jenkins_maze" / "indy_RTT") never match;
change the assignment in that loop to keep the full task identifier by setting
m["task"] = task and, if you still want the short label for session IDs, add a
new field such as m["task_short"] = task.split("_")[1] (or similar) so existing
dispatch in process() and the branches that look for full task names continue to
work.
- Around line 3-6: The dependency line using
"temporaldata@git+https://github.com/neuro-galaxy/temporaldata@main" should be
pinned to an immutable ref; replace the "@main" suffix with a specific tag or
commit SHA (e.g., "@vX.Y.Z" or "@<commit-sha>") in the dependency string in the
pipeline (the commented dependency entry) and make the matching change in the
test expectation referenced in tests/test_cli.py (the test that asserts the
dependency string at line ~59) so both the pipeline and the test use the same
pinned ref.
---
Outside diff comments:
In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py`:
- Around line 201-214: extract_trials() currently assumes trial_table has a
split_indicator column and unconditionally reads trial_table.split_indicator,
which breaks for RTT assets that lack predefined splits; to fix, guard the split
extraction by checking for the presence of the split_indicator column or
attribute on trial_table before using it (e.g., if "split_indicator" in
trial_table.columns or hasattr(trial_table, "split_indicator")) and only build
train_mask_nwb/test_mask_nwb when present, otherwise initialize
trials.train_mask_nwb and trials.test_mask_nwb to safe defaults (e.g., all False
or appropriate empty masks) so the RTT-specific branch can run without error;
update the code around Interval.from_dataframe(trial_table) and the assignments
to trials.train_mask_nwb and trials.test_mask_nwb accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9cc755d-0a24-47eb-b31e-16a5e2f1eb06
📒 Files selected for processing (2)
brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.pytests/test_cli.py
Comment on lines
+3
to
+6
| # dependencies = [ | ||
| # "dandi==0.74.0", | ||
| # "temporaldata@git+https://github.com/neuro-galaxy/temporaldata@main" | ||
| # ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
import urllib.request
def get(url: str):
with urllib.request.urlopen(url) as response:
return json.load(response)
repo = get("https://api.github.com/repos/neuro-galaxy/temporaldata")
default_branch = repo["default_branch"]
print(f"default_branch={default_branch}")
tags = get("https://api.github.com/repos/neuro-galaxy/temporaldata/tags")
print("tags=", [tag["name"] for tag in tags[:10]], sep="")
head = get(f"https://api.github.com/repos/neuro-galaxy/temporaldata/commits/{default_branch}")
print(f"{default_branch}_sha={head['sha']}")
PYRepository: neuro-galaxy/brainsets
Length of output: 2680
🏁 Script executed:
#!/bin/bash
# First, check the main file mentioned
echo "=== Checking pipeline.py lines 1-10 ==="
cat -n brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py | head -15
echo ""
echo "=== Checking tests/test_cli.py for temporaldata ==="
rg "temporaldata" tests/ --max-count=20Repository: neuro-galaxy/brainsets
Length of output: 856
🏁 Script executed:
#!/bin/bash
# Check the exact context in test_cli.py
echo "=== tests/test_cli.py around temporaldata reference ==="
grep -n "temporaldata" tests/test_cli.py
echo ""
echo "=== Full context around that line ==="
grep -B5 -A5 "temporaldata" tests/test_cli.pyRepository: neuro-galaxy/brainsets
Length of output: 784
Pin temporaldata to an immutable revision.
Using @main makes brainsets prepare non-reproducible and allows upstream branch moves to break older runs. Please pin a tag or commit SHA; tests/test_cli.py line 59 will need the same update.
🔒 Proposed fix
-# "temporaldata@git+https://github.com/neuro-galaxy/temporaldata@main"
+# "temporaldata@git+https://github.com/neuro-galaxy/temporaldata@<tag-or-commit-sha>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 3 - 6,
The dependency line using
"temporaldata@git+https://github.com/neuro-galaxy/temporaldata@main" should be
pinned to an immutable ref; replace the "@main" suffix with a specific tag or
commit SHA (e.g., "@vX.Y.Z" or "@<commit-sha>") in the dependency string in the
pipeline (the commented dependency entry) and make the matching change in the
test expectation referenced in tests/test_cli.py (the test that asserts the
dependency string at line ~59) so both the pipeline and the test use the same
pinned ref.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py (1)
188-216:⚠️ Potential issue | 🟠 MajorHandle missing
splitcolumn for RTT datasets.
extract_trialsunconditionally accessestrial_table.split_indicatorat lines 208-209. Since thesplitcolumn is renamed at line 198 and pandas silently ignores renaming non-existent columns, RTT datasets lacking asplitcolumn will cause aKeyErrorwhen accessing the non-existentsplit_indicatorcolumn.The masks are only used for the maze task (lines 165-169), making this check conditional is the correct approach:
Proposed fix
- # the dataset has pre-defined train/valid splits, we will use the valid split - # as our test - train_mask_nwb = trial_table.split_indicator.to_numpy() == "train" - test_mask_nwb = trial_table.split_indicator.to_numpy() == "val" - - trials.train_mask_nwb = ( - train_mask_nwb # Naming with "_" since train_mask is reserved - ) - trials.test_mask_nwb = test_mask_nwb # Naming with "_" since test_mask is reserved + # the dataset has pre-defined train/valid splits, we will use the valid split + # as our test (only available for maze, not RTT) + if "split_indicator" in trial_table.columns: + train_mask_nwb = trial_table.split_indicator.to_numpy() == "train" + test_mask_nwb = trial_table.split_indicator.to_numpy() == "val" + + trials.train_mask_nwb = ( + train_mask_nwb # Naming with "_" since train_mask is reserved + ) + trials.test_mask_nwb = test_mask_nwb # Naming with "_" since test_mask is reserved🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 188 - 216, In extract_trials, guard the creation of train_mask_nwb/test_mask_nwb against datasets that lack a split column: after renaming (trial_table = trial_table.rename(...)), check if "split_indicator" is in trial_table.columns; if present compute train_mask_nwb = trial_table.split_indicator.to_numpy() == "train" and test_mask_nwb = trial_table.split_indicator.to_numpy() == "val", otherwise set train_mask_nwb and test_mask_nwb to boolean arrays of length len(trial_table) filled with False (e.g., np.zeros(len(trial_table), dtype=bool)); then assign these to trials.train_mask_nwb and trials.test_mask_nwb as before so downstream code that expects those attributes won't crash.
🧹 Nitpick comments (4)
brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py (4)
155-155: Use idiomaticnot insyntax.
if "test" not in str(fpath):is more readable and follows Python conventions.♻️ Proposed fix
- if not "test" in str(fpath): + if "test" not in str(fpath):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` at line 155, Replace the non-idiomatic membership test in the conditional using "not" before the expression; change the condition that currently reads if not "test" in str(fpath): to use the preferred form if "test" not in str(fpath): so update the check around the fpath usage in pipeline.py (where the conditional appears) accordingly.
41-44: Consider annotating mutable class attribute.Static analysis flagged this dict as a mutable class attribute. While it's used read-only here, annotating with
ClassVarprevents accidental mutation and silences the linter.♻️ Proposed fix
+from typing import ClassVar + class Pipeline(BrainsetPipeline): brainset_id = "pei_pandarinath_nlb_2021" - dandiset_id = { + dandiset_id: ClassVar[dict[str, str]] = { "jenkins_maze": "DANDI:000140/0.220113.0408", "indy_RTT": "DANDI:000129/0.241017.1444", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 41 - 44, The dictionary dandiset_id is defined as a mutable class attribute; annotate it as a ClassVar to indicate it is intended to be a static, read-only class-level constant and silence linters. Import ClassVar from typing (if not already imported) and change the annotation for dandiset_id to use ClassVar[dict[str, str]] (or ClassVar[Dict[str, str]] if using Dict) so the class attribute is explicitly non-instance state; ensure the name dandiset_id remains unchanged and no runtime behavior is altered.
241-271: Unusedtrialsparameter (same asextract_behavior_maze).The
trialsparameter is not used in the function body. If both behavior extraction functions don't need trials, consider removing the parameter from both for consistency.♻️ Proposed fix (if parameter is not needed)
-def extract_behavior_rtt(nwbfile, trials): - +def extract_behavior_rtt(nwbfile): cursor_pos = nwbfile.processing["behavior"]["cursor_pos"].data[:]And update the call site at lines 176-178:
- data.cursor, data.finger, data.target = extract_behavior_rtt( - nwbfile, trials - ) + data.cursor, data.finger, data.target = extract_behavior_rtt(nwbfile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 241 - 271, The extract_behavior_rtt function declares an unused trials parameter (same issue exists for extract_behavior_maze); remove the trials parameter from the function signature of extract_behavior_rtt (and extract_behavior_maze if present) and update all call sites that pass trials (for example the calls around where extract_behavior_rtt is invoked) to call the functions without that argument so the signatures and invocations remain consistent.
219-238: Unusedtrialsparameter.The
trialsparameter is passed but never used. Consider removing it, or document that it's reserved for future use.♻️ Proposed fix (if parameter is not needed)
-def extract_behavior_maze(nwbfile, trials): +def extract_behavior_maze(nwbfile):And update the call site at line 159:
- data.hand, data.eye = extract_behavior_maze(nwbfile, trials) + data.hand, data.eye = extract_behavior_maze(nwbfile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 219 - 238, The function extract_behavior_maze currently declares a trials parameter that is never used; remove the unused parameter from the function signature (change def extract_behavior_maze(nwbfile, trials): to def extract_behavior_maze(nwbfile):) and update all call sites that pass trials to call extract_behavior_maze(nwbfile) instead (or, if trials is intended for future use, add a comment like "# reserved for future use" and prefix the param name with an underscore: _trials to signal it is intentionally unused). Ensure you update the function definition and every caller accordingly so names match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py`:
- Around line 188-216: In extract_trials, guard the creation of
train_mask_nwb/test_mask_nwb against datasets that lack a split column: after
renaming (trial_table = trial_table.rename(...)), check if "split_indicator" is
in trial_table.columns; if present compute train_mask_nwb =
trial_table.split_indicator.to_numpy() == "train" and test_mask_nwb =
trial_table.split_indicator.to_numpy() == "val", otherwise set train_mask_nwb
and test_mask_nwb to boolean arrays of length len(trial_table) filled with False
(e.g., np.zeros(len(trial_table), dtype=bool)); then assign these to
trials.train_mask_nwb and trials.test_mask_nwb as before so downstream code that
expects those attributes won't crash.
---
Nitpick comments:
In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py`:
- Line 155: Replace the non-idiomatic membership test in the conditional using
"not" before the expression; change the condition that currently reads if not
"test" in str(fpath): to use the preferred form if "test" not in str(fpath): so
update the check around the fpath usage in pipeline.py (where the conditional
appears) accordingly.
- Around line 41-44: The dictionary dandiset_id is defined as a mutable class
attribute; annotate it as a ClassVar to indicate it is intended to be a static,
read-only class-level constant and silence linters. Import ClassVar from typing
(if not already imported) and change the annotation for dandiset_id to use
ClassVar[dict[str, str]] (or ClassVar[Dict[str, str]] if using Dict) so the
class attribute is explicitly non-instance state; ensure the name dandiset_id
remains unchanged and no runtime behavior is altered.
- Around line 241-271: The extract_behavior_rtt function declares an unused
trials parameter (same issue exists for extract_behavior_maze); remove the
trials parameter from the function signature of extract_behavior_rtt (and
extract_behavior_maze if present) and update all call sites that pass trials
(for example the calls around where extract_behavior_rtt is invoked) to call the
functions without that argument so the signatures and invocations remain
consistent.
- Around line 219-238: The function extract_behavior_maze currently declares a
trials parameter that is never used; remove the unused parameter from the
function signature (change def extract_behavior_maze(nwbfile, trials): to def
extract_behavior_maze(nwbfile):) and update all call sites that pass trials to
call extract_behavior_maze(nwbfile) instead (or, if trials is intended for
future use, add a comment like "# reserved for future use" and prefix the param
name with an underscore: _trials to signal it is intentionally unused). Ensure
you update the function definition and every caller accordingly so names match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de8b556a-66e9-41d4-9ded-733e17df0af4
📒 Files selected for processing (1)
brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py
vinamarora8
reviewed
Mar 19, 2026
vinamarora8
reviewed
Mar 19, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py (3)
38-41: Consider adding aClassVarannotation to suppress the mutable default warning.The static analysis tool flags this as a mutable class attribute. While this is used as read-only configuration and is safe in practice, you can silence the warning with a type annotation.
♻️ Proposed fix
+from typing import ClassVar + class Pipeline(BrainsetPipeline): brainset_id = "pei_pandarinath_nlb_2021" - dandiset_id = { + dandiset_id: ClassVar[dict[str, str]] = { "jenkins_maze": "DANDI:000140/0.220113.0408", "indy_RTT": "DANDI:000129/0.241017.1444", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 38 - 41, Annotate the dandiset_id attribute with typing.ClassVar to indicate it’s an immutable class-level configuration and silence the mutable-default warning: import ClassVar and Dict from typing and change the dandiset_id declaration to use ClassVar[Dict[str, str]] while leaving the existing mapping values unchanged (refer to the dandiset_id symbol in this file).
238-268: Remove unusedtrialsparameter (same asextract_behavior_maze).The
trialsparameter is accepted but not used. Consider removing it for consistency.♻️ Proposed fix
-def extract_behavior_rtt(nwbfile, trials): - +def extract_behavior_rtt(nwbfile): cursor_pos = nwbfile.processing["behavior"]["cursor_pos"].data[:]Then update the call site at line 173:
- data.cursor, data.finger, data.target = extract_behavior_rtt( - nwbfile, trials - ) + data.cursor, data.finger, data.target = extract_behavior_rtt(nwbfile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 238 - 268, The function extract_behavior_rtt currently accepts an unused parameter trials; remove trials from its signature so it matches extract_behavior_maze (change def extract_behavior_rtt(nwbfile, trials): to def extract_behavior_rtt(nwbfile):) and update all call sites that pass trials to this function to stop supplying that argument (search for extract_behavior_rtt(...) usages). Ensure any tests or downstream callers are updated accordingly and run the pipeline to confirm no call-site remains passing the removed parameter.
216-235: Remove unusedtrialsparameter.The
trialsparameter is passed but never referenced in the function body. If it's reserved for future use or API consistency, consider adding_ = trialsor a comment; otherwise remove it.♻️ Proposed fix (if unused)
-def extract_behavior_maze(nwbfile, trials): +def extract_behavior_maze(nwbfile):Then update the call site at line 156:
- data.hand, data.eye = extract_behavior_maze(nwbfile, trials) + data.hand, data.eye = extract_behavior_maze(nwbfile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py` around lines 216 - 235, The function extract_behavior_maze declares an unused parameter trials; remove the parameter from the function signature (change def extract_behavior_maze(nwbfile, trials): to def extract_behavior_maze(nwbfile):) and update all call sites that pass trials to instead call extract_behavior_maze(nwbfile), or if you prefer to keep the signature for API compatibility, explicitly mark it unused inside the function (e.g., _ = trials or a comment) so linters and reviewers know it's intentional; refer to the extract_behavior_maze definition and its callers to apply the change consistently.
🤖 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/pei_pandarinath_nlb_2021/pipeline.py`:
- Around line 198-201: The code rounds trial timestamps via
Interval.from_dataframe(trial_table) then setting trials.end and trials.start
with np.round(..., 1), which discards sub-100ms precision; either stop doing
that rounding (remove the np.round calls) so original timestamps from
trial_table are preserved, or if the rounding is intentional, add an explanatory
comment next to the Interval.from_dataframe/trials.end and trials.start lines
documenting the rationale (e.g., to remove floating‑point noise or align to
100ms bins) and consider using a different precision if 0.1s is too coarse.
---
Nitpick comments:
In `@brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py`:
- Around line 38-41: Annotate the dandiset_id attribute with typing.ClassVar to
indicate it’s an immutable class-level configuration and silence the
mutable-default warning: import ClassVar and Dict from typing and change the
dandiset_id declaration to use ClassVar[Dict[str, str]] while leaving the
existing mapping values unchanged (refer to the dandiset_id symbol in this
file).
- Around line 238-268: The function extract_behavior_rtt currently accepts an
unused parameter trials; remove trials from its signature so it matches
extract_behavior_maze (change def extract_behavior_rtt(nwbfile, trials): to def
extract_behavior_rtt(nwbfile):) and update all call sites that pass trials to
this function to stop supplying that argument (search for
extract_behavior_rtt(...) usages). Ensure any tests or downstream callers are
updated accordingly and run the pipeline to confirm no call-site remains passing
the removed parameter.
- Around line 216-235: The function extract_behavior_maze declares an unused
parameter trials; remove the parameter from the function signature (change def
extract_behavior_maze(nwbfile, trials): to def extract_behavior_maze(nwbfile):)
and update all call sites that pass trials to instead call
extract_behavior_maze(nwbfile), or if you prefer to keep the signature for API
compatibility, explicitly mark it unused inside the function (e.g., _ = trials
or a comment) so linters and reviewers know it's intentional; refer to the
extract_behavior_maze definition and its callers to apply the change
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 039b0da5-5ffc-4938-b832-21ed73fecbc7
📒 Files selected for processing (1)
brainsets_pipelines/pei_pandarinath_nlb_2021/pipeline.py
vinamarora8
reviewed
Mar 19, 2026
Member
|
@divyansha1115 I will review this by Tuesday May 19! Sorry for the delay earlier |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.