User Testing of 2.1.0

[RayStick]

I have done a bit of user testing on 2.1.0 today. Apologies if these issues are covered in another Issue or PR somewhere - I don't think they are, but there has been a lot of activity recently so I may have missed it!

Firstly, it's great to see the code support multi-run files - well done all involved!

I called phys2bids with two different multi-run input files. One works, and one doesn't.

Expected Behavior

File 1
phys2bids -in four_scans_samefreq_allchannels.txt -chtrig 1 -ntp 534 400 24 513 -tr 1.2 1.2 4 1.2 -thr 2
It should split the files according to the -ntp listed (534, 400, 24, 513)
File 2
phys2bids -in Test2_samefreq_TWOscans.txt -chtrig 1 -ntp 534 513 -tr 1.2 1.2 -thr 2
It should split the files according to the -ntp listed (534, 513)

Actual Behavior

File 1
It does not split the files according the the -ntp given. It finds (534, 400, 6, 187). I tried different thresholds.
@smoia , I noticed this was the file that you previewed in your software demo video , and you also did not get the right number of time points (I know it was just a proof-of-principle thing for now).
It doesn't find the right number of time points for scan 3, therefore it fails for scan 4 as well?
But has anyone got this file to work properly?
File 2
This works, horray! As in, it splits the file with the correct number of time points.

Steps to Reproduce the Problem

1. Run the above commands for file1 and file2 (data is in OSF)
2. Look at the log output and the png files to see the problem.

Specifications

- Python version: Python 3.7.5
- phys2bids version: phys2bids v2.1.0
- Platform: MacOS

Possible solution

If no-one has got the four_scans file to work properly (to find the right time points) I recommend we do not test with this one initially. If you have, let me know how you did it please 😄 .

It is failing because the first two TRs for the third scan (24 volumes) have a different spacing - see picture below. This is an ASL scan, not a typical BOLD-fMRI scan, and I need to think about whether I want these two time points included in the main scan or not (as this is my data). Anyway, we do not have a way of dealing with this uneven TR spacing in the code yet, right? And we do not have a way of ignoring certain triggers. If so, I can make an issue for us to tackle this in the future, and we should use different multi-run scans for testing the code in the meantime.

[RayStick]

@sangfrois @smoia @rmarkello @vinferrer @eurunuela

[RayStick]

@eurunuela is the 'testing' label just for unit tests?

[eurunuela]

I'd say the multi-run option is not perfect atm. Maybe @smoia and @sangfrois can give us a more detailed insight.

The testing label is for PRs that add tests or issues that request tests . I don't think this is the case, but add it back if you think it is.

[RayStick]

Sorry, I should actually read the label descriptions ... !

[eurunuela]

No problem! It’s not very clear and the guidelines are not up to date so I’m not so sure either.

[sangfrois]

Hi @RayStick
Glad to read you are getting familiar with the new multirun option we pushed for in #36

Anyway, we do not have a way of dealing with this uneven TR spacing in the code yet, right?

No, we do not at the moment. thanks for addressing it.

And we do not have a way of ignoring certain triggers. If so, I can make an issue for us to tackle this in the future, and we should use different multi-run scans for testing the code in the meantime.

Same thing here, we do not have a way to deal with unwanted triggers as slice4phys uses check_trigger_amount method of Blueprintinput class, and that means trigger detection and manipulation is done at the same time. In practical terms : the program detects the first trigger then takes user's inputs to determine the end cutting point. i.e. there are no easy way, building up on phys2bids code, to ignore specific triggers (neither procedurally or manually).

I'm wondering : why would we have uneven TRs ? isn't that caused by experimenters who re-start the scanning period becuse of some accident or any other human error? If so, I wonder if @tsalo 's #219 could address such a bug

[tsalo]

I'm wondering : why would we have uneven TRs ? isn't that caused by experimenters who re-start the scanning period because of some accident or any other human error? If so, I wonder if @tsalo 's #219 could address such a bug

My approach doesn't look at TR (except to calculate duration of each scan), so it's possible that it could help.

[smoia]

Sorry - hard to follow convos in this period.

If no-one has got the four_scans file to work properly (to find the right time points) I recommend we do not test with this one initially. If you have, let me know how you did it please 😄 .

I didn't manage to make it work properly, and I agree we shouldn't be using this file to test! We don't support anything that is not even TR now.

I'd say the multi-run option is not perfect atm. Maybe @smoia and @sangfrois can give us a more detailed insight.

No part of phys2bids is "perfect". I'm sure there's always a better way to do whatever we're doing. The multi-run option is not not perfect either though - like any other part of phys2bids. It just has limited functionality that one day we'll expand.

I'm wondering : why would we have uneven TRs ? isn't that caused by experimenters who re-start the scanning period becuse of some accident or any other human error? If so, I wonder if @tsalo 's #219 could address such a bug

AFAIK the uneven TR can appear in ASL sequences like the one in this file. I'm not an expert but if I got it right it could be due to the tagging vs baseline? @RayStick can you explain us?

Let's be clear though, this is not a bug.

Anyway, we do not have a way of dealing with this uneven TR spacing in the code yet, right?

Exactly. check_trigger_amount (that we use to find the triggers and the timing of the scans) is not built to deal with uneven TR.
As a consequence, the splitting method we use now is not made to deal with uneven TR, since it relies heavily on check_trigger_amount.

For this reason, I don't think this issue is a "bug" though! If you want, the "bug" is that we don't state in the documentation that for the moment we don't support uneven TR. IT might be worth adding it in the readme and in the documentation.

And we do not have a way of ignoring certain triggers.

Nope, not yet. Should we?

If so, I can make an issue for us to tackle this in the future, and we should use different multi-run scans for testing the code in the meantime.

Yes, please, do so! It would be great to have issues to track features we should integrate to support more data that we aren't supporting yet!

No problem! It’s not very clear and the guidelines are not up to date so I’m not so sure either.

Please do feel free to open PRs or issues to propose clarifications for labels, guidelines and documentation in general! The earlier the better - since we're going to adopt the same labels across repositories.

I have done a bit of user testing on 2.1.0 today.

If you want to leave more extensive notes, remember you have blueprints to document user testing. Here's the reference

[RayStick]

Thanks for the comments @sangfrois, @tsalo and @smoia

Stefano, I agree that this is not a bug if we are not meant to be supporting files that (1) have uneven TRs and (2) have TRs that we want to ignore, at the moment. And I completely forget about the UserTesting file - sorry! Will use that in future for notes like this.

I plan to:

1. Make a few issues related to enhancements that could be good for future (support uneven TRs, ignore TRs) - I don't think this is a big priority though.
2. Update the documentation to make it clear we do not support that at the moment.

By the way, I think the progress made on the multi-run option is great!
My only concern was that we were testing it with the four_scans file which is not going to work at the moment (sorry, I should have realized this before sharing this file).

As to why we could have uneven TRs:

You would not expect to have this in a standard BOLD-fMRI sequence. However, with Arterial Spin Labelling (ASL) sequence there are a few reasons. It is not due to tagging vs baseline (as they should be evenly spaced) it is due to some prescans or prep scans that need to run before the main ASL sequence. Often we run a “calibration scan” for the purposes of perfusion quantification: normally a single volume with the same acquisition parameters but without tagging the blood and with a really long TR so magnetization is fully relaxed. This sequence also happens to be a 3D segmented ASL sequence, which makes the timings and the triggers even weirder.

[RayStick]

And just to point out (which I may not have been clear about before): the errors I got where to do with using the the four_scans file, and not to do with the multi-run implementation itself i.e. if the problematic scan3 was in a file on its own (one run) the code will still fail to process it.

[RayStick]

I am closing this issue now because:

(1) I have updated the user testing documentation here
(2) I have made a new issue here

There are a few small documentation changes I would suggest, that I will add to an existing issue/make a new issues about.