Fixes v1 vs v2 curriculum differences - #174
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the duplicated CI test execution and strengthen the two-session boundary tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Aligns v1/v2 curriculum behavior, stage identifiers, block-length distributions, tests, schemas, and CI discovery.
Changes:
- Requires two sessions before uncoupled Stage 2 progression.
- Adjusts block-length distributions and generated schemas.
- Updates stage-name tests and CI test discovery.
File summaries
| File | Summary |
|---|---|
workspace/aind_behavior_dynamic_foraging_curricula/tests/test_uncoupled.py |
Updates stage names and transition tests; boundary case should use 2 sessions. |
workspace/aind_behavior_dynamic_foraging_curricula/tests/test_uncoupled_baiting.py |
Updates stage names and transition tests; boundary case should use 2 sessions. |
workspace/aind_behavior_dynamic_foraging_curricula/tests/test_metrics.py |
Updates stage-name fixtures. |
workspace/aind_behavior_dynamic_foraging_curricula/tests/test_coupled_baiting.py |
Updates stage-name assertions. |
workspace/aind_behavior_dynamic_foraging_curricula/src/aind_behavior_dynamic_foraging_curricula/uncoupled/stages.py |
Adjusts block-length distributions. |
workspace/aind_behavior_dynamic_foraging_curricula/src/aind_behavior_dynamic_foraging_curricula/uncoupled/curriculum.py |
Adds the two-session Stage 2 requirement. |
workspace/aind_behavior_dynamic_foraging_curricula/src/aind_behavior_dynamic_foraging_curricula/uncoupled_baiting/stages.py |
Adjusts block-length distributions. |
workspace/aind_behavior_dynamic_foraging_curricula/src/aind_behavior_dynamic_foraging_curricula/uncoupled_baiting/curriculum.py |
Adds the two-session Stage 2 requirement. |
workspace/aind_behavior_dynamic_foraging_curricula/src/aind_behavior_dynamic_foraging_curricula/coupled_baiting/stages.py |
Adjusts block-length distributions. |
schema/uncoupled.json |
Regenerated uncoupled schema. |
schema/uncoupled_baiting.json |
Regenerated uncoupled baiting schema. |
schema/coupled_baiting.json |
Regenerated coupled baiting schema. |
.github/workflows/dynamic-foraging-cicd.yml |
Expands test discovery; currently duplicates workspace test execution. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "scaling_parameters": { | ||
| "scale": 1.0, | ||
| "offset": 10.0 | ||
| } |
There was a problem hiding this comment.
I'm confused on the logic here.
- Before we were sampling between 10 and 20, now we are sampling between 0 and 20 and then adding 10, resulting in 10-30
- So shouldn't we instead sample between 0 and 10, and then add 10?
| block_length=ExponentialDistribution( | ||
| distribution_parameters=ExponentialDistributionParameters(rate=0.2), | ||
| truncation_parameters=TruncationParameters(min=10, max=20), | ||
| truncation_parameters=TruncationParameters(truncation_mode="clamp", min=0, max=20), |
There was a problem hiding this comment.
same question here, I don't follow the logic of how the parameterization works
|
@alexpiet I implemented the offset the same way for the iti period in #150. Looking at the v1 implementation of the block length here and the v1 of iti period here, it seemed like the same logic applied. In #150, I validated that when given the same seed, these two methods worked identically. I've done the same validation with the block length, but I might have missed something 
|
alexpiet
left a comment
alexpiet
left a comment
There was a problem hiding this comment.
Thanks for the validation!
|
@arjunsridhar12345 I'm not sure if anything needs to change on your side for this if you already implemented the min/offset logic for the iti |
Fixes v1 vs v2 curriculum differences
resolves #200
resolves #201
resolves #202