cpp-lln-lab / bidspm

an SPM centric BIDS app
https://bidspm.readthedocs.io/en/latest/general_information.html
GNU General Public License v3.0
5 stars 13 forks source link

[FIX] small fixes in createDefaultStatsModel.m, getAcquisitionTime.m & bidsResults.m #1248

Closed d-ni374 closed 2 months ago

d-ni374 commented 2 months ago

Small fixes in 3 scripts:

sourcery-ai[bot] commented 2 months ago

🧙 Sourcery has finished reviewing your pull request!


Tips - Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - You can change your review settings at any time by accessing your [dashboard](https://sourcery.ai/dashboard): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.
Remi-Gau commented 2 months ago

Thanks @d-ni374 for the PR!!!! And sorry for being slow at responding.

I will add a few tests to your PR to try to make sure that those problems do not come back in the future.

Remi-Gau commented 2 months ago

@all-contributors please add @d-ni374 for bug, code

allcontributors[bot] commented 2 months ago

@Remi-Gau

I've put up a pull request to add @d-ni374! :tada:

Remi-Gau commented 2 months ago

To check:

seems that the results workflow may have been affected by this:

https://github.com/cpp-lln-lab/bidspm/actions/runs/9805840957/job/27076371017?pr=1258#step:17:1284

[09:15:02] bidspm - INFO                printProcessingSubject
  PROCESSING SUBJECT No.: 1 SUBJECT LABEL : 01
  List of contrast in this SPM file

    - listening_1
    - listening_inf_baseline_1

  [Warning:
  [09:15:02] bidspm - WARNING               getContrastNb

  This SPM file
  /tmp/tp9fcf8405_d635_4ef5_9125_7a7133955b2f/outputs/derivatives/bidspm-stats/sub-01/task-auditory_space-individual_FWHM-6/SPM.mat
   does not contain a contrast named:
   "listening_1_[0-9]+"
  ] 
Remi-Gau commented 2 months ago

To check:

seems that the results workflow may have been affected by this:

https://github.com/cpp-lln-lab/bidspm/actions/runs/9805840957/job/27076371017?pr=1258#step:17:1284

[09:15:02] bidspm - INFO              printProcessingSubject
  PROCESSING SUBJECT No.: 1 SUBJECT LABEL : 01
  List of contrast in this SPM file

      - listening_1
      - listening_inf_baseline_1

  [�Warning:
  [09:15:02] bidspm - WARNING             getContrastNb

  This SPM file
  /tmp/tp9fcf8405_d635_4ef5_9125_7a7133955b2f/outputs/derivatives/bidspm-stats/sub-01/task-auditory_space-individual_FWHM-6/SPM.mat
   does not contain a contrast named:
   "listening_1_[0-9]+"
  ]� 

Ok this happens on octave and matlab and can reproduce locally. I must have missed something and don't have proper tests to detect this.

Will open a new issue to track this new bug.

Remi-Gau commented 2 months ago

Still missed something:

warning: 
[08:14:52] bidspm - WARNING                             getContrastNb

This SPM file
/outputs/ds000001/derivatives/bidspm-stats/sub-02/task-balloonanalogrisktask_space-MNI152NLin2009cAsym_FWHM-0/SPM.mat
 does not contain a contrast named:
 "cash_demean_[0-9]+"

https://app.circleci.com/pipelines/github/cpp-lln-lab/bidspm/887/workflows/2e3f5eb5-8af7-4644-a4f2-8f6ed55c4845/jobs/2733

d-ni374 commented 2 months ago

I've seen that the contrast names for ds000001 end with 'run-01'. Maybe changing the regular expression for identifying run level contrasts in the function endsWithRunNumber to `'[-][0-9]+\${0,1}$'` may solve the problem.

Remi-Gau commented 2 months ago

I've seen that the contrast names for ds000001 end with 'run-01'. Maybe changing the regular expression for identifying run level contrasts in the function endsWithRunNumber to `'[-][0-9]+\${0,1}$'` may solve the problem.

yup having a look at it now

also fixing some other issues I introduced in other PRs...

Remi-Gau commented 2 months ago

OK things were a bit more complicated because contrasts need to be named differently depending on if the datasets uses sessions or use run-entities:

so I updated the documentation to explain it: see this PR https://bidspm--1273.org.readthedocs.build/en/1273/stats/bids_stats_model.html#run-level

The regex got more complex: .*(_[0-9]+\$?$|_(?:ses|run)-[0-9]*$|_ses-[0-9]*_run-[0-9]*$)

To make it more clear, here are the test cases I am using:

**  % no run label or session label could be inferred from bids names
  assertEqual(endsWithRunNumber('foo_1'), true);
  assertEqual(endsWithRunNumber('foo_10'), true);

  % session label but not run label could be inferred from bids names
  % the session has only one run with no run label in the file name
  assertEqual(endsWithRunNumber('foo_ses-1'), true);
  assertEqual(endsWithRunNumber('foo_ses-29'), true);

  % only run label could be inferred from bids names
  % there is only one session
  % but it is not used explicitly in the filenames
  assertEqual(endsWithRunNumber('foo_run-1'), true);
  assertEqual(endsWithRunNumber('foo_run-05'), true);

  % subject level contrast names should have nothing appended
  assertEqual(endsWithRunNumber('foo'), false);
  assertEqual(endsWithRunNumber('foo_1_'), false);
  assertEqual(endsWithRunNumber('foo_1_a'), false);
  assertEqual(endsWithRunNumber('foo_'), false);

  % there should be an underscore
  assertEqual(endsWithRunNumber('foo1'), false);