Horizon updates b1 #1965
Horizon updates b1 #1965
Conversation
… horizon_updates_b
|
Hello @Garyfallidis, Thank you for updating ! Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated at 2019-12-06 22:46:49 UTC |
…o horizon_updates_b1
There was a problem hiding this comment.
Hi @Garyfallidis, Thank you for this! here my first feedback.
I still have to play with Horizon and I have some questions. But I need to play more with Horizon before asking. Can you fix some pep8 about line length too?
Thank you
dipy/viz/app.py
Outdated
| @@ -4,15 +4,15 @@ | |||
| from dipy.io.stateful_tractogram import Space, StatefulTractogram | |||
There was a problem hiding this comment.
pep8: not used, you can remove it
dipy/viz/app.py
Outdated
| @@ -4,15 +4,15 @@ | |||
| from dipy.io.stateful_tractogram import Space, StatefulTractogram | |||
| from dipy.io.streamline import save_tractogram | |||
There was a problem hiding this comment.
pep8: not used, you can remove it
dipy/viz/app.py
Outdated
| @@ -138,90 +139,29 @@ def __init__(self, tractograms=None, images=None, pams=None, | |||
| self.tractogram_clusters = {} | |||
| self.recorded_events = recorded_events | |||
| self.show_m = None | |||
| # self.mem = GlobalHorizon() | |||
There was a problem hiding this comment.
Can you remove this comment, it appears below
| @@ -108,6 +108,7 @@ def __init__(self, tractograms=None, images=None, pams=None, | |||
| out_png : string | |||
| recorded_events : string | |||
| File path to replay recorded events | |||
| return_showm : bool | |||
There was a problem hiding this comment.
Can you improve documentation here, a lot of options, we need informations
dipy/viz/app.py
Outdated
|
|
||
| def add_actors(self, scene, tractograms, threshold): | ||
| def add_actors(self, scene, tractograms, threshold, callbacks=True): |
There was a problem hiding this comment.
- Can you document arguments of this function?
- Same comment as above: update function name:
add_streamlines_actors\add_bundle_actor\ ...
dipy/viz/gmem.py
Outdated
| @@ -31,9 +33,11 @@ def __init__(self): | |||
|
|
|||
| # tractogram level sharing | |||
| self.cluster_thr = 15 | |||
| # self.cluster_lengths = [] # not used | |||
There was a problem hiding this comment.
needs 2 spaces between [] and #
dipy/viz/gmem.py
Outdated
| @@ -31,9 +33,11 @@ def __init__(self): | |||
|
|
|||
| # tractogram level sharing | |||
| self.cluster_thr = 15 | |||
| # self.cluster_lengths = [] # not used | |||
| # self.cluster_sizes = [] # not used | |||
There was a problem hiding this comment.
needs 2 spaces between [] and #
dipy/viz/gmem.py
Outdated
| @@ -31,9 +33,11 @@ def __init__(self): | |||
|
|
|||
| # tractogram level sharing | |||
| self.cluster_thr = 15 | |||
| # self.cluster_lengths = [] # not used | |||
| # self.cluster_sizes = [] # not used | |||
| # self.cluster_thr_min_max = [] # not used | |||
There was a problem hiding this comment.
needs 2 spaces between [] and #
| affine = np.diag([2., 1, 1, 1]).astype('f8') | ||
| data = 255 * np.random.rand(150, 150, 150) | ||
| images = [(data, affine)] | ||
| # images = None |
dipy/viz/tests/test_apps.py
Outdated
| recorded_events=fname) | ||
| # npt.assert_equal(os.path.exists('tmp.trk'), True) | ||
| # npt.assert_equal(os.stat('tmp.trk').st_size > 0, True) | ||
| # os.remove('tmp.trk') |
There was a problem hiding this comment.
comments to remove or uncomment
There was a problem hiding this comment.
Thanks for this @Garyfallidis. it will be good if you could add more documentation in general. Many parameters on the command line are not documented. Otherwise, Can you fix the pep8 from codacy?
You can find my first comments below. Review still in progress...
| options = [r'un\hide centroids', 'invert selection', | ||
| r'un\select all', 'expand clusters', | ||
| 'collapse clusters', 'save streamlines', | ||
| 'recluster'] |
There was a problem hiding this comment.
'recluster' option is not visible on the panel. Is it voluntary?
| VisibilityOn() | ||
| c.VisibilityOff() | ||
| self.cea[c]['expanded'] = 1 | ||
| if key == 'r' or key == 'R': |
There was a problem hiding this comment.
reset() unhide hidden clusters? it will be good to collapse only the visible cluster
|
|
|
This PR looks good to me. If there is no other comment until wednesday I will go ahead and merge it. |
|
@skoudoro : I see that there are still a couple of comments you made that are not addressed. |
|
Indeed, but I believe now that they deserve a new, smallest and focus PR. |
|
thank you @Garyfallidis! merging and waiting for new PR |
Testing mesa and xvfb issues