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

[NF]: Add seeds to TRK #1792

Merged
merged 7 commits into from Jun 12, 2019

Conversation

@AntoineTheb
Copy link
Contributor

commented Mar 19, 2019

Seeds can now be saved alongside their respective streamlines using the data_per_streamline dictionary of the Tractogram class, using seeds as a key. Seed return from the LocalTracker class is optional so as to prevent breaking changes to pre-existing scripts.

This solves #1791.


Example usage:

from dipy.io.streamline import TrkFile
from dipy.viz import window, actor

# Load files and data
trk = TrkFile.load(args.tractogram)
tractogram = trk.tractogram
streamlines = tractogram._streamlines
seeds = tractogram.data_per_streamline['seeds']

# Make display objects
streamlines_actor = actor.line(streamlines)
points = actor.dots(seeds, color=(1., 1., 1.))

# Add display objects to canvas
s = window.Scene()
s.add(streamlines_actor)
s.add(points)

window.show(s)

Which should output something like
image

@pep8speaks

This comment has been minimized.

Copy link

commented Mar 19, 2019

Hello @AntoineTheb, Thank you for updating !

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

Comment last updated at 2019-05-17 20:09:29 UTC
NF: Add seeds to TRK
Seeds can now be saved alongside their respective streamlines using the
`data_per_streamline` dictionary of the Tractogram class.

@AntoineTheb AntoineTheb force-pushed the AntoineTheb:atheb/save_seeds_in_trk branch from 557cc58 to f4d9f8e Mar 20, 2019

@AntoineTheb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@skoudoro @Garyfallidis Seems like the test that failed has nothing to do with the current PR (the test is about DKI modeling). Should I raise an issue ?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

I think there is already an issue. I have just restarted the failing Travis matrix.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 22, 2019

Codecov Report

Merging #1792 into master will decrease coverage by 0.68%.
The diff coverage is 93.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1792      +/-   ##
==========================================
- Coverage   84.28%   83.59%   -0.69%     
==========================================
  Files         117      118       +1     
  Lines       14222    14505     +283     
  Branches     2245     2290      +45     
==========================================
+ Hits        11987    12126     +139     
- Misses       1718     1852     +134     
- Partials      517      527      +10
Impacted Files Coverage Δ
dipy/workflows/tracking.py 96.34% <100%> (+0.39%) ⬆️
dipy/tracking/utils.py 88.99% <100%> (+0.13%) ⬆️
dipy/tracking/local/localtracking.py 95.87% <81.81%> (-1.88%) ⬇️
dipy/core/gradients.py 90% <0%> (-0.61%) ⬇️
dipy/workflows/flow_runner.py 100% <0%> (ø) ⬆️
dipy/pkg_info.py 28.57% <0%> (ø) ⬆️
dipy/io/pickles.py 100% <0%> (ø) ⬆️
dipy/utils/six.py 42.44% <0%> (ø)
dipy/align/imwarp.py 98.39% <0%> (ø) ⬆️
dipy/reconst/dki.py 96.82% <0%> (ø) ⬆️
... and 14 more
@AntoineTheb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@skoudoro Any update on this ? What do you think ?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Hi @AntoineTheb,

This is definitively a nice feature! I need to play a bit with it before reviewing.

it will be good to have other people opinion too like @arokem @guaje @gabknight, ...

When you save the seeds, is the TRK file working correctly with other software like Trackvis?
It would be good to add an option on save_trk and load_trk function or to create a user-friendly load/save function

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

To clarify my last sentence, we should be able to load/save the seeds without running a tracking algorithm

@AntoineTheb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Thanks ! I just checked it and everything seems normal with Trackvis. Also, it does seem like a good idea to be able to save only the seeds/save them separately. I'll get on it right way.

@skoudoro skoudoro added this to the 1.0 milestone Apr 23, 2019

@arokem

This comment has been minimized.

Copy link
Member

commented May 7, 2019

This looks like a good addition to me. +1 to adding I/O options, but that can happen on a follow-up PR.

@arokem

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Looks like there are a few conflicts introduced when #1766 was merged. Could you please resolve these, @AntoineTheb ? Thanks!

@arokem
Copy link
Member

left a comment

Thanks - this is great! I had a couple more comments/questions, but they're fairly minor

dipy/tracking/utils.py Outdated Show resolved Hide resolved
dipy/tracking/utils.py Outdated Show resolved Hide resolved
Add test and fix `move_streamlines`
- Add test to make sure that seeds and streamlines are in the same space
- Change `move_streamlines` to fix awkward streamlines tuple

@AntoineTheb AntoineTheb force-pushed the AntoineTheb:atheb/save_seeds_in_trk branch from 07e9d79 to 26fee32 May 7, 2019

@AntoineTheb

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@arokem @skoudoro Looks like a single build has failed during the CI run. Could it be possible to retry it ? Thanks !

@arokem

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Hey @AntoineTheb : Is this ready to go from your point of view? Would you mind rebasing on the master branch once again?

@AntoineTheb

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@arokem Sure, no problem ! And yes I think this is ready to go, extra features like I/O options can be added at a later time

@AntoineTheb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

@arokem @skoudoro Any updates on this ? I think the CI errors are unrelated to this PR

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

As soon as there is new PR for I/O options, I am ok with this PR

@arokem arokem merged commit 7f4bfd2 into nipy:master Jun 12, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Hi @AntoineTheb : sorry for the slowness here. OHBM has been holding us back.

I think that this is ready to go. I'll post an issue about I/O and assign that to you.

1 similar comment
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Hi @AntoineTheb : sorry for the slowness here. OHBM has been holding us back.

I think that this is ready to go. I'll post an issue about I/O and assign that to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.