fix: respect user numTargets and dimensions in IED morphobes task - #1
Merged
Conversation
checkInput was unconditionally overwriting task-specific defaults for task='ied', so a user's numTargets=4 became 2 and edDimension='appendage' became 'shape'. Only apply IED defaults to fields the caller did not explicitly supply, and let 4D sizing follow numTargets. - add clutil.normaliseDimension to accept plural/case variants (appendages -> appendage) and reject invalid dimensions - robust stage parsing for the GUI array-literal taskType format - warn when the ED dimension has constant levels in set 3 (EDS/EDR) - delete unused startIEDmorphobes4D.m; behaviour now lives in the unified startIEDmorphobes.m with numTargets=4 - fix ClutilTest to expect checkInput default task='train' - add regression tests for overrides, normalisation and stage parsing
📝 WalkthroughWalkthroughIED input handling now preserves caller values, normalizes dimensions, parses GUI task stages, and warns about constant ED dimensions. The 4D morphobes task file was removed. Tests cover the updated defaults and task behavior. ChangesIED task configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant checkInput
participant startIEDmorphobes
participant normaliseDimension
Caller->>checkInput: Provide IED settings
checkInput->>checkInput: Apply missing defaults
checkInput-->>startIEDmorphobes: Return configured inputs
startIEDmorphobes->>normaliseDimension: Normalize ID and ED dimensions
normaliseDimension-->>startIEDmorphobes: Return canonical dimensions
startIEDmorphobes->>startIEDmorphobes: Parse task stages and validate ED levels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @+clutil/checkInput.m:
- Around line 124-141: Constrain IED target counts before applying defaults: in
the fixed-variant branch for task values {'ied-2' 'ied-4'}, assign the computed
n back to in.numTargets before calling applyIedDefaults so each variant always
uses its required count. In the classic 'ied' branch, validate the supplied or
default n and reject any value other than 2 or 4 before applyIedDefaults; add
regression coverage for both fixed variants and an unsupported classic target
count.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5950d3d3-32d2-45bf-9bc5-94b5a7fc708d
📒 Files selected for processing (6)
+cltasks/startIEDmorphobes.m+cltasks/startIEDmorphobes4D.m+clutil/checkInput.m+clutil/normaliseDimension.mtests/ClutilTest.mtests/StartIEDMorphobesTest.m
💤 Files with no reviewable changes (1)
- +cltasks/startIEDmorphobes4D.m
Comment on lines
+124
to
+141
| % Classic 'ied' defaults to 2D, but if the caller explicitly | ||
| % sets numTargets=4 then 4D sizing follows. | ||
| n = 2; | ||
| if isfield(in, 'numTargets') && ~isempty(in.numTargets) | ||
| n = in.numTargets; | ||
| end | ||
| if n == 4 | ||
| in = applyIedDefaults(in, userFields, 4, 8, 12); | ||
| else | ||
| in = applyIedDefaults(in, userFields, 2, 10, 15); | ||
| end | ||
| case {'ied-2' 'ied-4'} | ||
| in.taskType = 'sd cd cr ids idr eds edr'; % stages run in sequence | ||
| in.idDimension = 'colour'; % 'shape','colour','appendage','texture' — ID dim | ||
| in.edDimension = 'shape'; % 'shape','colour','appendage','texture' — ED dim | ||
| in.criterion = 6; % consecutive correct to advance | ||
| in.maxIncorrect = 50; % incorrect trials on stage before task terminates | ||
| in.objectSize = 8; % size of objects in degrees | ||
| in.objectSep = 12; % separation of objects in degrees | ||
| in.sampleY = 0; % vertical centre of the 2x2 grid in degrees | ||
| in.trialTime = 5.0; % max trial time in seconds | ||
| in.targetHoldTime = 0.2; % target hold time in seconds | ||
| in.morphobesFolder = ''; % morphobes dataset folder (defaults to resources/morphobes) | ||
| in.fixSize = 2; % fixation size in degrees | ||
| in.fixWindow = 4; % fixation window size in degrees | ||
| if strcmp(in.task, 'ied-2') | ||
| in.numTargets = 2; % 2D variant: two targets (left/right) | ||
| n = 2; | ||
| else | ||
| in.numTargets = 4; % 4D variant: four targets in 2x2 grid | ||
| n = 4; | ||
| end | ||
| in = applyIedDefaults(in, userFields, n, 8, 12); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constrain numTargets and enforce fixed IED variants.
applyIedDefaults preserves a supplied numTargets. Therefore, task='ied-2' with numTargets=4 returns four targets, and task='ied-4' with numTargets=2 returns two targets.
Classic ied also preserves unsupported values such as numTargets=3. clutil.iedMorphobesConfig accepts only 2 or 4, so this fails later during task initialization.
Set in.numTargets = n before applyIedDefaults for ied-2 and ied-4. Validate that classic ied accepts only 2 or 4. Add regression tests for both fixed variants and an unsupported target count.
Proposed fix
case {'ied-2' 'ied-4'}
if strcmp(in.task, 'ied-2')
n = 2;
else
n = 4;
end
+ in.numTargets = n;
in = applyIedDefaults(in, userFields, n, 8, 12);Also applies to: 169-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @+clutil/checkInput.m around lines 124 - 141, Constrain IED target counts
before applying defaults: in the fixed-variant branch for task values {'ied-2'
'ied-4'}, assign the computed n back to in.numTargets before calling
applyIedDefaults so each variant always uses its required count. In the classic
'ied' branch, validate the supplied or default n and reject any value other than
2 or 4 before applyIedDefaults; add regression coverage for both fixed variants
and an unsupported classic target count.
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
1 participant
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.
Bug Description
Running
cltasks.startIEDmorphobeswithnumTargets=4andedDimension='appendages'still produced two targets using colour+shape dimensions.Root Cause
Two bugs in the input pipeline:
clutil.checkInputunconditionally overwrote user fields. Fortask='ied'(which the CageLab GUI always sends), checkInput forcednumTargets=2,idDimension='colour',edDimension='shape', plus criterion/maxIncorrect/taskType. Any caller-supplied values were silently discarded before the task ran.validDims = {'shape','colour','appendage','texture'}rejected the plural'appendages'(and mixed case), falling back to'shape'.A third latent bug: the GUI's Task Order field uses a MATLAB array-literal format
'[ "sd" "sr" ... ]'that the old stage parser would have collapsed.Fix
+clutil/checkInput.m— captureuserFieldsbefore applying defaults; a newapplyIedDefaultshelper only fills fields the caller didn't supply.numTargets=4now survives (with 4D sizing 8/12),numTargets=2keeps 2D sizing (10/15); criterion/maxIncorrect/taskType overrides are respected too.+clutil/normaliseDimension.m(new) — maps plural/case/whitespace variants to canonical names ('appendages'→'appendage'), returns''for invalid dimensions.+cltasks/startIEDmorphobes.m— usesnormaliseDimension; robust stage parser that strips[ ] " ' , ;from GUI-style taskType; warns (ConstantEDDimension) when the chosen ED dimension has no variance in set 3 (e.g. appendage ED withnumTargets=2), pointing the user to 4D.+cltasks/startIEDmorphobes4D.m— deleted (unused; 4D behaviour lives in the unifiedstartIEDmorphobes.mvianumTargets=4).tests/ClutilTest.m— fixed pre-existing failure: checkInput default is'train'(changed in cf8a245), test expected'generic'.tests/StartIEDMorphobesTest.m— 8 new regression tests (user overrides preserved,normaliseDimensionedge cases, ED-dimension shift usage, GUI taskType parsing, constant-ED warning).How to Verify
clutil.checkInput(struct('task','ied','numTargets',4))→numTargets=4, objectSize=8, objectSep=12clutil.checkInput(struct('task','ied','numTargets',4,'edDimension','appendages'))→ preservedrelDim = in.edDimension)Test Plan
Risk Assessment
Low — input-default handling and dimension validation only; no changes to trial loop, stimulus selection, or data logging. The
checkInputbehaviour change means previously-stomped user overrides now take effect, which is the intended fix.Summary by CodeRabbit
New Features
Bug Fixes