Skip to content

[FIX] repaired convolution issue that arises when onset values are repeated#1133

Merged
effigies merged 2 commits intobids-standard:masterfrom
jmumford:fix-sample-condition-step
May 28, 2025
Merged

[FIX] repaired convolution issue that arises when onset values are repeated#1133
effigies merged 2 commits intobids-standard:masterfrom
jmumford:fix-sample-condition-step

Conversation

@jmumford
Copy link
Copy Markdown
Contributor

@jmumford jmumford commented May 22, 2025 •
edited by effigies
Loading

Michael discovered and odd behavior if onset values were repeated (e.g. two stimuli occurred at the same time). I traced the problem to _sample_condition. Basically, it creates a boxcar regressor and the duplicate onset times break the code when the block needs to start or end. The repair I did basically adopts what nilearn currently uses for the same function, so I'm assuming they identified and repaired this bug a while ago.

Note, I've only fixed this issue as far as convolution is concerned. I'm not sure if this problem arises elsewhere.

The following reproduces the problem I'm referring to.

from bids.modeling.hrf import compute_regressor
import numpy as np
import matplotlib.pyplot as plt

onset = [16, 24, 48, 79, 80, 112, 127, 144, 176, 176, 208, 216, 240,
         272, 272, 304, 329]
duration = [16, 1, 16, 1, 16, 16, 1, 16, 1, 16, 16, 1, 16, 1, 16, 16, 1]

trial_types = {
    'Face': [0., 0., 1., 0., 0., 1., 0., 0., 0., 1., 0., 0., 1., 0., 0., 1., 0.],
    'Hand': [1., 0., 0., 0., 1., 0., 0., 1., 0., 0., 1., 0., 0., 0., 1., 0., 0.],
    'Swallow': [0., 1., 0., 1., 0., 0., 1., 0., 1., 0., 0., 1., 0., 1., 0., 0., 1.]
}

model = 'spm'
resample_frames = np.arange(0, 340.0, 0.1)

for name, trial_type in trial_types.items():
    vals = np.vstack([onset, duration, trial_type])
    print(vals.transpose())
    regressor = compute_regressor(
        vals, model, resample_frames, fir_delays=None,
        min_onset=0, oversampling=1
    )
    plt.plot(regressor[0], label=name)

plt.legend()
plt.show()

What pybids currently produces

wrong_result_current_pybids

What you get with my fix

correct_result_with_jmumford_fix

@codecov
Copy link
Copy Markdown

codecov bot commented May 22, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.73%. Comparing base (376d420) to head (f6d2e30).
Report is 5 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   89.71%   89.73%   +0.01%     
==========================================
  Files          65       66       +1     
  Lines        7205     7219      +14     
  Branches     1137     1140       +3     
==========================================
+ Hits         6464     6478      +14     
  Misses        540      540              
  Partials      201      201

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@effigies effigies force-pushed the fix-sample-condition-step branch from e157d10 to 8159c30 Compare May 24, 2025 14:03
@effigies
Copy link
Copy Markdown
Collaborator

effigies commented May 24, 2025

Resolved the test failure issues. Could you add a test? I think adapting your code to validate that the regressor values are all between -0.2 and 1.2 should do the job.

We should create a test_hrf.py inside src/bids/modeling/tests, and it can look something like:

import numpy as np

from ..hrf import compute_regressor


def test_duplicate_onsets():
    [your code]

    assert np.all((regressor > 0.2) & (regressor < 1.2))

@effigies effigies merged commit 26b3f7e into bids-standard:master May 28, 2025
28 checks passed
effigies added a commit that referenced this pull request Sep 24, 2025
@effigies
Merge tag '0.20.0'
a61732c 
Version 0.20.0 (September 24, 2025)

New feature release in the 0.20.x series.

This release finalizes some deprecations that were started in the 0.7 and 0.14 series.
In particular, passing indexer arguments to ``BIDSLayout`` and disabling
``absolute_paths`` mode in ``BIDSLayout()`` or ``BIDSLayout.get()`` will now error.
To adjust indexing behavior, create an explicit ``BIDSLayoutIndexer()`` and pass
``BIDSLayout(..., indexer=indexer)``.
To access relative paths, use the ``BIDSFile.relative_path`` property on
``BIDSLayout.get()`` return values.

* FIX: Check for attribute that can be absent before assigning (#1165)
* FIX: Accommodate deprecations in the upcoming Pandas 3 release (#1173)
* FIX: Repaired convolution issue that arises when onset values are repeated (#1133)
* ENH: StatsModels: Add `invalid_inputs` parameter to allow dropping of missing data columns to run nodes (#1145)
* ENH: Add missing root-level path patterns (#1169)
* ENH: Parse ``seg-`` entities from dseg or probseg files (#1172)
* DOC: Update link to derivatives config file (#1132)
* MNT: Address warnings produced by pybids and dependencies (#1136)
* MNT: Lock test environment, update with dependabot (#1134)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jmumford @effigies