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

Transform points with DiffeormorphicMap #2369

Merged
merged 8 commits into from Sep 27, 2022

Conversation

skoudoro
Copy link
Member

@skoudoro skoudoro commented Apr 13, 2021

This PR is just a follow-up and close #936. Many people ask for this feature and we never merged it.

it adds 2 functions to apply warp fields on point: transform_points and transform_points_inverse. After registering two images, you can use this function to transform your streamlines/points.

You might be interested @vigji @wjy73 @dPys. Feel free to test this PR and give feedback/review. Thanks in advance!

fixes #2327
fixes #2313

TODO

  • Create or Improves the current tutorial.
  • Need to compare with deform_streamlines in dipy.tracking.streamline before merging. Performance should be better. Results should be similar.
  • After comparison, remove duplicate? or make sure it uses the same codebase?

@pep8speaks
Copy link

pep8speaks commented Apr 13, 2021

Hello @skoudoro, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2021-10-31 18:54:02 UTC

@skoudoro skoudoro added this to the 1.5.0 milestone Apr 21, 2021
@skoudoro skoudoro linked an issue Jul 27, 2021 that may be closed by this pull request
@skoudoro skoudoro mentioned this pull request Oct 29, 2021
23 tasks
@arokem
Copy link
Contributor

arokem commented Oct 29, 2021

I think that we should go ahead and merge this, adding a documentation example on a separate PR.

@arokem
Copy link
Contributor

arokem commented Oct 29, 2021

@skoudoro : I believe the CI failures are unrelated (and probably fixed in #2413?)

Would you mind rebasing this one?

@skoudoro
Copy link
Member Author

thank you @arokem. rebase done. I will try to write a tutorial this week. If I don't succeed, I will create an issue as a reminder and you will be able to merge this.

@codecov
Copy link

codecov bot commented Oct 31, 2021

Codecov Report

Merging #2369 (4d9c9bc) into master (08b8993) will decrease coverage by 0.05%.
The diff coverage is 54.83%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2369      +/-   ##
==========================================
- Coverage   84.24%   84.19%   -0.06%     
==========================================
  Files         130      130              
  Lines       17919    17948      +29     
  Branches     3053     3059       +6     
==========================================
+ Hits        15096    15111      +15     
- Misses       2106     2118      +12     
- Partials      717      719       +2     
Impacted Files Coverage Δ
dipy/align/imwarp.py 95.56% <53.33%> (-2.87%) ⬇️
dipy/tracking/streamline.py 93.80% <100.00%> (ø)

@arokem
Copy link
Contributor

arokem commented Nov 1, 2021

OK - looks like everything is passing now.

Looks like some of the new code is not currently tested. Do you want to use deform_streamlines to test it?

That could bring us a step closer towards unifying the code base across these two functions.

@alexrockhill
Copy link
Contributor

How's this going? Looks like maybe ready to merge, happy to help write an example/documentation if that's of any use.

@skoudoro
Copy link
Member Author

Hi @alexrockhill,

Thank you for pinging me on this PR.

The last step is to compare with deform_streamlines in dipy.tracking.streamline and to update the tests. Just need to find time to finalize this.

Concerning tutorials and documentation, it can be done on another PR. Thank you for proposing your help and feel free to do a PR over my branch in my fork.

@alexturcea
Copy link

Hello @skoudoro , is there any plan for merging this?

@skoudoro
Copy link
Member Author

Hi @alexturcea,

I will have time this month to finalize this PR. So it will be on the release this month.

Hi @arokem,

I compared deform_streamlines and transform_points and we get the exact same results. Slightly better performance with transform_points. I added a test on 1a90b2b

if all CI's come back green, this is ready to go.

@skoudoro
Copy link
Member Author

PS: I will update and create a tutorial on a new PR

@skoudoro skoudoro changed the title [WIP] Transform points with DiffeormorphicMap Transform points with DiffeormorphicMap Sep 18, 2022
@skoudoro skoudoro mentioned this pull request Sep 20, 2022
22 tasks
@skoudoro
Copy link
Member Author

I will wait until wednesday, and if no news, I will go ahead and merge this PR, is that ok @arokem and @Garyfallidis?

@arokem
Copy link
Contributor

arokem commented Sep 26, 2022

Yeah: 👍 from my end.

@Garyfallidis Garyfallidis merged commit 556a755 into dipy:master Sep 27, 2022
@Garyfallidis
Copy link
Contributor

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants