Repository navigation
Fix tuningcurve calculator dealing over a single stimulus property - #1029
Merged
Merged
Conversation
When exactly one property was dealt, the deal index was set to 1 on every iteration, so all N_deal tuning curves were built at the first value of the dealt property. Use the loop index instead. Also accumulate deal_log_str rather than restarting it from deal_str. Adds testDealSingleProperty to testCalcTuningCurve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F7s7BWxMrHf4PtsMiWS3sx
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1029 +/- ##
==========================================
+ Coverage 31.63% 31.66% +0.02%
==========================================
Files 898 898
Lines 46907 46907
==========================================
+ Hits 14841 14855 +14
+ Misses 32066 32052 -14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stevewds
approved these changes
Oct 15, 2026
stevevanhooser
pushed a commit
to Waltham-Data-Science/NDI-python
that referenced
this pull request
Oct 15, 2026
tuningcurve: NDI-matlab 49a875c94 (VH-Lab/NDI-matlab#1029) fixed the single-property deal index in calculate; the Python calculate is still a stub with no deal loop, so nothing to port. Recorded in decision_log. downloadDocumentCollection: NDI-matlab 2669a8525 (the paired claude/download-timeout-60s branch) raises the default Timeout to 60 s, which this PR ports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T41FUDkgY9DAstG7LzZHaV
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
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
ndi.calc.stimulus.tuningcurvecan "deal" a stimulus property (selection(i).value = 'deal'), which makes one tuning curve per value of that property. When exactly one property was dealt, the deal loop set the index to 1 on every iteration:So the calculator still produced N_deal curves, but every one was built at the first value of the dealt property.
History
Commit dcf9952 ("small changes for Nielsen", 2025-04-15) added this single-property branch. It replaced the unconditional
eval(['[' deal_str ']=ind2sub(deal_constraints_dim,i);']);from 60e0da8 (2023-10-16). Before dcf9952, dealing over one property gave one curve per value.Any tuning curve made with exactly one dealt property between dcf9952 and this fix was built at the first value of that property, and needs to be recomputed. Calculations with no dealt property, or with two or more, are unaffected.
Evidence (real data)
One stimulus presentation had 8 spatial frequencies (0.04 to 1.25 cpd) × 6 contrasts × 4 angles. Contrast tuning curves dealt over
sFrequencycame out as 8 identical curves: label Contrast, values[0.04 0.08 0.16 0.32 0.64 1], stimid[97;98;99;100;101;102], all at sFrequency 0.04. Downstream,ndi.calc.vis.contrast_sensitivity(VH-Lab/NDIcalc-vis-matlab) then reportedspatial_frequenciesas[0.04 x 8].Changes
src/ndi/+ndi/+calc/+stimulus/tuningcurve.msz_{1} = i;(deal_stris'sz_{1}'in this case, so this is the same as using the loop index). With no dealt property,deal_constraints{1} = 1and N_deal = 1, so i is always 1 and nothing changes.deal_log_strinstead of restarting fromdeal_str(the'sz_{1},...'eval string). Each curve's log now lists every property dealt for it.tests/+ndi/+unittest/+calc/+stimulus/testCalcTuningCurve.m: newtestDealSingleProperty. It usesndi.mock.fun.stimulus_responseto build a mock 3 contrast × 4 spatial_frequency stimulus set. It then runs the calculator for contrast tuning withspatial_frequencydealt and checks that:tuningcurve_calc.stim_property_list.valuesis different, and together they match the dealt valuesdealing spatial_frequency = <value>Testing
MATLAB was not available where this change was written (see AGENTS.md), so it was not run locally. In CI on 49a875c, "Run tests (MATLAB latest)" passed: 1556 tests, +1 vs base (the new
testDealSingleProperty), 0 failures. I did not run the new test against the unfixed code to confirm it fails there.🤖 Generated with Claude Code
https://claude.ai/code/session_01F7s7BWxMrHf4PtsMiWS3sx