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

WIP: Det track workflow and other improvements in workflows #1324

Closed
wants to merge 44 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Garyfallidis
Member

Garyfallidis commented Aug 30, 2017

This a new PR that tries to correct issues with previous workflow PRs and puts down the necessary frameworks for the RecoBundles interfaces.
The main focus here is to provide a simple to use tracking workflow (a work that @matthieudumont started) however I realized that there are other important areas that need polishing together with the tracking workflow. I am providing some fixes and updates here. Still some work needed.

@arokem

Nice! A few questions and comments for now

def run(self, input_files, out_dir=''):
""" Workflow for creating a binary mask

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

Probably a copy-paste?

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 30, 2017

Member

Still a WIP I haven't polished it yet.

"""
#io_it = self.get_io_iterator()
#logging.basicConfig(format='',)

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

You can remove these

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 30, 2017

Member

Actually do you know how to remove INFO from logging?

if os.path.basename(input_path).find('bvals') > -1:
bvals = np.loadtxt(input_path)
logging.info('Bvalues \n {0}'.format(bvals))
bvals = np.array([0, 1000, 2000, 3000, 3040, 2060], dtype='f8')

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

Where did these come from?

This comment has been minimized.

@Garyfallidis
@@ -54,6 +55,8 @@ def run(self, input_files, bvalues, bvectors, mask_files, b0_threshold=0.0,
multiple masks at once. (default: No mask used)
b0_threshold : float, optional
Threshold used to find b=0 directions (default 0.0)
bvecs_tol : float, optional
Threshold used so that norm(bvec)=1 (default 0.01)

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

I am not sure I understand this explanation.

@@ -216,6 +225,8 @@ def run(self, input_files, bvalues, bvectors, mask_files, sigma,
An estimate of the variance.
b0_threshold : float, optional
Threshold used to find b=0 directions (default 0.0)
bvecs_tol : float, optional
Threshold used so that norm(bvec)=1 (default 0.01)

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

Same comment here

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

Looks like maybe this input doesn't get used in this function?

@@ -454,18 +479,20 @@ def run(self, input_files, bvalues, bvectors, mask_files,
bvals, bvecs = read_bvals_bvecs(bval, bvec)
gtab = gradient_table(bvals, bvecs,
b0_threshold=b0_threshold)
b0_threshold=b0_threshold, atol=bvecs_tol)

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

It occurs to me: would it make more sense to use atol instead of bvecs_tol for consistency with the gradients module?

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 15, 2017

Member

atol is not really informative especially from the command line.

sh_order = 8
sh_order = 6
"""

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

If you are going to triple-string the code below, maybe remove it?

class DetTrackPAMFlow(GenericTrackFlow):
@classmethod
def get_short_name(cls):
return 'tracking'

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

Maybe you want different names for these different flows? They are currently all named "tracking"

stopping_thr=0.2,
seed_density=1,
use_sh=False,
out_dir='',

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

I must be missing something: I don't see where out_dir gets used.

This comment has been minimized.

@Garyfallidis
def run(self, peaks_values, peaks_idxs, peaks_dirs, stopping_files,
seeding_files, stopping_thr=0.2, seed_density=1, out_dir='',
out_tractogram='tractogram.trk'):

This comment has been minimized.

@arokem

arokem Aug 30, 2017

Member

Does out_tractogram get used?

This comment has been minimized.

@Garyfallidis
@arokem

This comment has been minimized.

Member

arokem commented Aug 30, 2017

Yup yup. Apologies if some of the comments are premature here! Looks great overall!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 30, 2017

No, it is not great. That's why the WIP. Thanks for the quick feedback. Will start cleaning up and continue correcting different issues. The 3 track flows are not needed at this stage however @matthieudumont did made them. Perhaps they are useful to the SCIL team. More asap.

@arokem

This comment has been minimized.

Member

arokem commented Aug 31, 2017

LOL. Looking forward to see how you eliminate them :-)

@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.308% when pulling 4eccbcd on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.308% when pulling 4eccbcd on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.308% when pulling 4eccbcd on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.308% when pulling 4eccbcd on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.308% when pulling 4eccbcd on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.308% when pulling 4eccbcd on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.308% when pulling 4eccbcd on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 18, 2017

@arokem I have some issue here with pytables. I need tables to work in all bots as pam5 needs tables. Can you have a look? There is something in the .travis file that I do not set correctly and getting a couple of bots failing. Any feedback much appreciated.

@coveralls

This comment has been minimized.

coveralls commented Sep 18, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.316% when pulling 001b042 on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Sep 18, 2017

Is this still an issue? It looks as though tests are passing on most of the bots.

It looks like pytables is intentionally not installed on this one: https://travis-ci.org/nipy/dipy/jobs/276673673, but error handling needs to be adjusted to handle the situation where pytables is not present.

On this one: https://travis-ci.org/nipy/dipy/jobs/276673666, it looks like maybe the minimal version of pytables needs to be increased?

@arokem

This comment has been minimized.

Member

arokem commented Sep 18, 2017

And the last one is probably #1323

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 18, 2017

Hi Ariel, thanks for looking into this.
In this one https://travis-ci.org/nipy/dipy/jobs/276673666 I think tables is not installed at all.
However, it is in the list of depends. Do we need some other lib for this package?
Currently, it is being installed using pytables.

On this one https://travis-ci.org/nipy/dipy/jobs/276673673 I want to have tables installed. I do not want to start skipping tests of workflows.

@arokem

This comment has been minimized.

Member

arokem commented Sep 18, 2017

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 18, 2017

Yes, I cannot see a way around it. We need it for saving/loading the peaks and tracks. I have already added 3.2.0 in tables. To select the version I went to the pytables website and selected something that was not the most recent version (about 2 years old). 3.2.0 seemed like a good solid number and supported in Ubuntu. But now I see that in Ubuntu the supported version is 3.2.2-2 (in xenial) and 3.1.1 (in trusty). I can try 3.1.1 and see what happens.

@coveralls

This comment has been minimized.

coveralls commented Sep 18, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.315% when pulling 5b165f3 on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 18, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.315% when pulling 5b165f3 on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 18, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.315% when pulling 5b165f3 on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 18, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.315% when pulling 5b165f3 on Garyfallidis:det_track_workflow into e498e68 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Sep 20, 2017

If I understand the error correctly, it means that the version of pytables that you are using (3.3) was compiled against another version of numpy. I think that you will need to change either the pytables minimal version or the numpy minimal version. The current version of pytables seems to require numpy 1.8.1 (http://www.pytables.org/usersguide/installation.html), so that might be the issue here as well (we have 1.7 as our minimal requirement).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 28, 2017

Okay I am switching to h5py. Has numpy 1.7.1 as minimum requirement version and I see you use it for other things too in travis. @arokem let me know if you disagree.

@arokem

This comment has been minimized.

Member

arokem commented Sep 28, 2017

No objection on my end. Seems solid.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 15, 2017

Rebased in #1358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment