-
Notifications
You must be signed in to change notification settings - Fork 429
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
Update of the tutorial apply image-based registration to streamlines #2930
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2930 +/- ##
==========================================
- Coverage 81.73% 81.68% -0.05%
==========================================
Files 146 146
Lines 20361 20375 +14
Branches 3232 3232
==========================================
+ Hits 16642 16644 +2
- Misses 2901 2913 +12
Partials 818 818
|
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.
Good stuff! I have one suggestion for refactoring some duplication in the code and another small comment about usage in the example, but overall - I believe this can go in.
streamlines._offsets = points._offsets.astype(old_offsets_dtype) | ||
streamlines._data = out.astype(old_data_dtype) | ||
return streamlines | ||
|
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.
While we're here, could we maybe reduce some of the duplication in this code? Unless I am missing something, these two functions are identical except for a couple of lines, so maybe we could move all of the code into one private function to be called from two different wrapper functions?
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.
That's a good point, I will do it tomorrow.
Co-authored-by: Ariel Rokem <arokem@gmail.com>
code refactored! this is ready to go @arokem. Thank you again for the suggestion, I do not know why I did not do that in the first place. |
I believe the CI failure is not a real thing, so merging. |
The goal of this PR is to fix a tutorial that give wrong result.
Also, we got many times this question so it was tie to fix the tutorial.
This is a follow up of #2369
it should resolves #2703, resolves #2400 and resolves #2786
it should answer #2646 discussion.
Sorry for the delay @WilliamFCB, it would be great if you could test it.