Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

ChengranAA
Copy link

@ChengranAA ChengranAA commented Dec 13, 2023

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.

@ofgulban
Copy link
Owner

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 ofgulban self-assigned this Dec 15, 2023
@ChengranAA
Copy link
Author

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

@ofgulban
Copy link
Owner

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

@ofgulban
Copy link
Owner

ofgulban commented Mar 21, 2024

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
Copy link
Owner

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 ofgulban closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants