Damn... I think there's still something wrong.

My multi-session test

• session 1: t1w & asl

• session 2: t1w, flair, & asl

• session 3: only asl

• Log for release branch: testLogRelease.txt

• Log for bugfix branch: testLogBugFix.txt

Result in bugfix branch

The folders sub-001_1, sub-001_2, & sub-001_3 are there. In sub-001_1 there is only the asl, in sub-001_2 there is the asl and the flair, & in sub-001_3 there is only the asl again... 🤔

Within DCM2NII we seem to find the correct sessions & modalities:

Matching files:
    'sub-001/session_1_abc/ASL/S811203'
    'sub-001/session_1_abc/T1w/S811205'
    'sub-001/session_2_abc/ASL/S811203'
    'sub-001/session_2_abc/FLAIR/S811207'
    'sub-001/session_2_abc/T1w/S811205'
    'sub-001/session_3_abc/ASL/S811203'

@jan-petr

Test set-up

Test DCM2NII

Here everything seems to work fine. There are a few warnings about missing scans, but the results are perfectly fine.

Test NII2BIDS

Here it already seems to go wrong. The T1 images are missing.

Looks good. My only question is why we set max sessions /visits to 0 by default, shouldn't this default to 1?

Also, i see that in your test case, you miss having a session with Anat but without ASL. I would test this as well.

Looks good. My only question is why we set max sessions /visits to 0 by default, shouldn't this default to 1?

nMaxVisits and nMaxSessions - not something we changed in this commit. And I can't find this parameter to be used/described anywhere else in the code. AFAIK, we can remove it completely - perhaps make an issue for that and do that in the next release (to be on the safe side).

Also, i see that in your test case, you miss having a session with Anat but without ASL. I would test this as well.

Yes. Not totally related, but doesn't hurt testing as well. @MichaelStritt - can you run your ADNI test again with a non-ASL session as well?

Looks good. My only question is why we set max sessions /visits to 0 by default, shouldn't this default to 1?

nMaxVisits and nMaxSessions - not something we changed in this commit. And I can't find this parameter to be used/described anywhere else in the code. AFAIK, we can remove it completely - perhaps make an issue for that and do that in the next release (to be on the safe side).

Also, i see that in your test case, you miss having a session with Anat but without ASL. I would test this as well.

Yes. Not totally related, but doesn't hurt testing as well. @MichaelStritt - can you run your ADNI test again with a non-ASL session as well?

nMaxVisits and nMaxSessions were added by Michael I believe in a commit in this PR.
"not totally related" -> I just noticed that Michael left out this test-case:)

Very happy otherwise. Most important thing is to keep stuff modular, the code should be as independent as pragmatically possible.

nMaxVisits and nMaxSessions were added by Michael I believe in a commit in this PR.

I think he just moved stuff around to a subfunction - he did not touch any functionality - just added checks and warnings. Can you confirm Michael - and then create an issue to 1.9.0 to remove the variables...

"not totally related" -> I just noticed that Michael left out this test-case:)

Yes - I agree, useful to check it to avoid future issues.

Very happy otherwise. Most important thing is to keep stuff modular, the code should be as independent as pragmatically possible.

Yes - agree!!

@HenkMutsaerts Can you approve? Because we don't plan changes, just Michael will test stuff and make a new issue...

Give me 10 more minutes, I've been improving the unit test this morning 😄

It contains a BIDS2Legacy test with 5 sessions now and a few other things have been improved.

I think he just moved stuff around to a subfunction - he did not touch any functionality - just added checks and warnings. Can you confirm Michael - and then create an issue to 1.9.0 to remove the variables...

Can confirm 👍

Edit: I can do the merge as soon as @HenkMutsaerts approves the PR 👍

Edit: Okay, added it to the release branch directly f1ababc . From my point of view everything is fine. Let's merge and do release testing 👍

Agree. Henk, please click the button. And we'll run the full-tests after merging everything (no risk with merging to release branch without all tests).