bbfrederick / rapidtide

rapidtide - a suite of programs for doing time lag correlation analysis on fMRI data
Apache License 2.0
75 stars 14 forks source link

--acfix in rapidtide does not work #115

Closed themeo closed 1 year ago

themeo commented 1 year ago

Describe the bug After running rapidtide from a singularity container (2.6.0), I get the following error:


Pass number 1 checking reference regressor autocorrelation properties searching for sidelobes with amplitude > 0.1 with abs(lag) < 30.0 s

WARNING: check_autocorrelation found bad sidelobe at 10.414616713318386 seconds (0.09601889608872338 Hz)... Removing sidelobe subjecting lag times to modulus removing spectral component at sidelobe frequency Traceback (most recent call last): File "/usr/local/miniconda/bin/rapidtide", line 23, in rapidtide_workflow.rapidtide_main(rapidtide_parser.process_args(inputargs=None)) File "/usr/local/miniconda/lib/python3.11/site-packages/rapidtide/workflows/rapidtide.py", line 1528, in rapidtide_main transferfunc=optiondict["transferfunc"],


KeyError: 'transferfunc'

To Reproduce This is my call: singularity exec /home/language/jaksze/rapidtide-2.6.0.simg rapidtide epi.nii.gz output --filterband lfo --passes 3 --nprocs 24 --noprogressbar --acfix

I know it is an experimental option, but I found some other experimental options having a positive impact on the SNR, so I tried also this one.

bbfrederick commented 1 year ago

When I say an option is experimental, I mean that it may or may not be beneficially scientifically or might not work as intended - it's not a license to crash! In any case, it's not that experimental - I've found it beneficial - I should move it out of that section.

I found and (I think) fixed the bug - I phased out specifying the transfer function for the filter a while back, and I guess I missed that call. I removed the offending line, and that should fix it, but I don't seem to have any data lying around with problematic sidelobes. I'll push this as 2.6.1 - the new container will probably be available on Dockerhub in about a half hour (the container takes about 20 minutes to build and push).

themeo commented 1 year ago

Thanks! And sure, I will let you know whether this worked.

On Thu, Aug 17, 2023 at 4:33 PM Blaise deB Frederick < @.***> wrote:

When I say an option is experimental, I mean that it may or may not be beneficially scientifically or might not work as intended - it's not a license to crash! In any case, it's not that experimental - I've found it beneficial - I should move it out of that section.

I found and (I think) fixed the bug - I phased out specifying the transfer function for the filter a while back, and I guess I missed that call. I removed the offending line, and that should fix it, but I don't seem to have any data lying around with problematic sidelobes. I'll push this as 2.6.1 - the new container will probably be available on Dockerhub in about a half hour (the container takes about 20 minutes to build and push).

— Reply to this email directly, view it on GitHub https://github.com/bbfrederick/rapidtide/issues/115#issuecomment-1682397089, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAS6PPUKP6IYF3ENZQNIHYLXVYTUFANCNFSM6AAAAAA3TDFM3U . You are receiving this because you authored the thread.Message ID: @.***>

bbfrederick commented 1 year ago

Container is building now.

themeo commented 1 year ago

I confirm there is now no error when using the option, thanks for the fix! The computation takes much longer than with other options but that I assume is expected.

On Thu, Aug 17, 2023 at 4:49 PM Blaise deB Frederick < @.***> wrote:

Container is building now.

— Reply to this email directly, view it on GitHub https://github.com/bbfrederick/rapidtide/issues/115#issuecomment-1682421831, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAS6PPRCV5KGRYQGL27G6D3XVYVODANCNFSM6AAAAAA3TDFM3U . You are receiving this because you authored the thread.Message ID: @.***>

bbfrederick commented 1 year ago

Glad to hear it!

About computation time - I wouldn't think it would take THAT much longer, since this is just a filtering operation when preparing the regressor - that's not a very computationally intensive step. Although maybe it's doing a lot more despeckling now, which can up the processing time. If you include a runtimings.txt file with and without, we could see where it's spending its time.

themeo commented 1 year ago

You are right, when I reran it, the difference was only 17% longer when including --acfix.

bbfrederick commented 1 year ago

Glad it worked!