bvbabel icon indicating copy to clipboard operation
bvbabel copied to clipboard

Matrix reader for affine and slice timings in fmr.py using numpy.fromstring method

Open ChengranAA opened this issue 2 years ago • 4 comments

Both the loop-based method and the NumPy-based method can achieve the same end results, the numpy implementation achieve slightly better speed and maintain a pythonic simple profile without additional while or for loops.

ChengranAA avatar Dec 13 '23 17:12 ChengranAA

Looks ok. I am not sure about the speed gain as this is a handful of numbers, have you measured?

TODO: I need to test this with the test fmrs, then good to merge.

ofgulban avatar Dec 15 '23 15:12 ofgulban

I did run some benchmarks on these two methods by generating some large string lists (n = 1000). The performance for numpy.fromsting is indeed faster but overall performance gain is quite small.

Benchmark: 
Numpy:  9.39e-05
Loops:  1.88e-04
Difference:  9.42e-05

ChengranAA avatar Dec 15 '23 17:12 ChengranAA

Cool, thanks. Next, I will test against some test data and merge. Probably sometime next week. If not early Jan.

ofgulban avatar Dec 15 '23 18:12 ofgulban

I will come back to this very soon. Just wanted to check with some other additional fmrs. Did not forget @ChengranAA , also I need to think for a a moment in combination with #44 as both are fmr related.

ofgulban avatar Mar 21 '24 11:03 ofgulban

Dear @ChengranAA , upon further consideration, I have decided to reject this pull request because of code readability reasons. Although your suggestion is very smart and shorthand way of reading the matrix information, I have decided that it would be more beneficial to have more easily readable code for future maintenance.

However, I appreciate your effort and therefore added you to the list of contributors in readme. Please do not shy away from submitting pull requests in the future (e.g. adding new functionality, or providing script examples).

ofgulban avatar May 28 '24 10:05 ofgulban