ciftify icon indicating copy to clipboard operation
ciftify copied to clipboard

Published 20 hours ago • verified publisher icon edickie

[FIX] Add Soroush's falff fixes

Open DESm1th opened this issue 2 years ago • 1 comments

It seems like the main fix happening is the use of repetition time as the second argument to np.fft.fftfreq on line 178, instead of 1. The two tests in tests/functional/test_ciftify_falff.py pass both before and after the changes. Unfortunately, I'm not sure precisely how to update them to capture what these changes are fixing.

DESm1th avatar Nov 08 '22 23:11 DESm1th

I think the major error here is that repetition time should never be hard coded.

We want it as an extra argument - or read from the file meta data.

I think I do this already in other scripts (cifti_clean?) so maybe we can just add that in?

On Tue, Nov 8, 2022 at 6:18 PM Dawn E. Smith @.***> wrote:

It seems like the main fix happening is the use of repetition time as the second argument to np.fft.fftfreq on line 178, instead of 1. The two tests in tests/functional/test_ciftify_falff.py pass both before and after the changes. Unfortunately, I'm not sure precisely how to update them to capture what these changes are fixing.

You can view, comment on, or merge this pull request online at:

https://github.com/edickie/ciftify/pull/177 Commit Summary

File Changes

(1 file https://github.com/edickie/ciftify/pull/177/files)

Patch Links:

  • https://github.com/edickie/ciftify/pull/177.patch
  • https://github.com/edickie/ciftify/pull/177.diff

— Reply to this email directly, view it on GitHub https://github.com/edickie/ciftify/pull/177, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADEXT5VP473TVFNZ6AAGLWDWHLNSRANCNFSM6AAAAAAR23BB24 . You are receiving this because your review was requested.Message ID: @.***>

edickie avatar Nov 09 '22 15:11 edickie