-
Notifications
You must be signed in to change notification settings - Fork 58
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
Optimize locating coordinates in convert_neurosynth_to_dataset #682
Conversation
Thanks for opening this pull request! We have detected this is the first time you have contributed to NiMARE. Please check out our contributing guidelines.
Of course, if you want to opt out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed. |
Codecov Report
@@ Coverage Diff @@
## main #682 +/- ##
=======================================
Coverage 85.24% 85.25%
=======================================
Files 41 41
Lines 4479 4482 +3
=======================================
+ Hits 3818 3821 +3
Misses 661 661
Continue to review full report at Codecov.
|
@ryanhammonds Thank you for the contribution! It looks like the linter is complaining, though. Can you run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me, and I was able to reproduce the profiling results locally. @adelavega, since you opened the original issue, I wanted to see if this is what you were thinking of before merging.
Also, @ryanhammonds, do you want to add yourself to the Zenodo file? |
Thanks @tsalo! No need to add me to the Zenodo file. Maybe if I make larger contributions in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. LGTM. I'm a bit surprised how much of a difference using numpy instead of pandas makes. Good lesson for other parts of the package.
Great, thanks @adelavega! Merging now. Thanks again for the contribution @ryanhammonds. |
Closes #666.
Changes proposed in this pull request:
convert_neurosynth_to_dataset
runtime from ~7min to ~3min.