Show spheres with different radii, colors and opacities + add timers + add exit a + resolve issue with imread #1528

Garyfallidis · 2018-05-20T14:43:36Z

Nice and simple to use function for visualizing many spheres with different radii and colors. RGB and RGBA colors are allowed. A (opacity) component can be different for each sphere.
Adding timers in ShowManager for building animations.
Allowing exiting (closing) a window from inside ShowManager.
Resolving issue with imread as it is not available anymore in scipy.misc.

pep8speaks · 2018-05-20T14:43:38Z

Hello @Garyfallidis, Thank you for updating !

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

Comment last updated on September 01, 2018 at 02:13 Hours UTC

ranveeraggarwal · 2018-05-20T15:36:11Z

dipy/viz/actor.py

+
+    glyph = vtk.vtkGlyph3D()
+    glyph.SetSourceConnection(src.GetOutputPort())
+    if major_version <= 5:


As mentioned by @nilgoyette in #1522, we should decide on an alternate name for this variable. Like vtk_major_version.

Sure but this is a topic for a different PR and see here #1544 Support of old vtk versions should be removed all together. More of a problem than a solution.

ranveeraggarwal · 2018-05-20T15:38:28Z

Looks good! A couple of points:

Let's merge the changes made by @thechargedneutron before we merge this in.
It will be great if we can have an example for this in doc/examples.

…i and opacities

iniital changes extra env files deleted input parametrs changed doc changed unnecessary venv files removed NF: added timer callback in ShowManager TEST: added test for timer callbacks TEST: Corrected execution order in test_timer DOC: new example showing the use of timer callbacks Minor changes DOC: correcting figure output Bad indent removed

codecov-io · 2018-08-09T19:42:47Z

Codecov Report

Merging #1528 into master will increase coverage by 0.01%.
The diff coverage is 70.87%.

@@            Coverage Diff             @@
##           master    #1528      +/-   ##
==========================================
+ Coverage   87.33%   87.34%   +0.01%     
==========================================
  Files         246      246              
  Lines       32164    32244      +80     
  Branches     3497     3500       +3     
==========================================
+ Hits        28089    28163      +74     
- Misses       3242     3246       +4     
- Partials      833      835       +2

Impacted Files	Coverage Δ
dipy/viz/window.py	`64.8% <30.76%> (+0.57%)`	⬆️
dipy/viz/tests/test_ui.py	`82.14% <76.66%> (-0.4%)`	⬇️
dipy/viz/tests/test_actors.py	`73.96% <84.61%> (+0.29%)`	⬆️
dipy/viz/actor.py	`82.73% <91.17%> (+0.01%)`	⬆️
dipy/tracking/tests/test_localtrack.py
dipy/io/tests/test_utils.py	`100% <0%> (ø)`
dipy/viz/utils.py	`61.67% <0%> (+5.98%)`	⬆️
dipy/io/utils.py	`53.12% <0%> (+28.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1e1f2e...5cb4f8a. Read the comment docs.

Garyfallidis · 2018-08-18T01:35:10Z

After travis passes @ranveeraggarwal, @karandeepSJ, @dmreagan and @MarcCote this should be ready to be merged.

MarcCote

I like the new timer feature. I've added some comments related to that.

MarcCote · 2018-08-20T12:54:00Z

dipy/viz/tests/test_ui.py

+    showm.initialize()
+
+    def timer_callback(obj, event):
+        global cnt, sphere_actor, showm, tb


Do you really need global here? Since timer_callback is a nested function, those variables should be in the scope.

MarcCote · 2018-08-20T12:55:42Z

dipy/viz/tests/test_ui.py

+
+    showm.initialize()
+
+    def timer_callback(obj, event):


What's obj here? What kind of VTK object should we expect?

MarcCote · 2018-08-20T12:59:44Z

doc/examples/viz_timers.py

+
+
+def timer_callback(obj, event):
+    global cnt, sphere_actor, showm, tb


I think we should avoid globals whenever it is possible. Here you can safely remove all globals and make cnt a mutable variable (e.g. a list). Even better, declare a counter = itertools.count() instead of cnt = 0 (at line 40), then replace cnt += 1 with cnt = next(counter) (at line 46).

MarcCote · 2018-08-20T13:02:48Z

doc/examples/viz_timers.py

+# Run every 200 milliseconds
+showm.add_timer_callback(True, 200, timer_callback)
+
+showm.start()


This should be commented?

Nope, start does not need to be commented @MarcCote :) That is the great thing here we can close the application whenever we want from inside the timer callback. Perfect for testing viz, building animations etc. This is a really nice new feature. Don't you agree? :)

Oh nice! ☺️

MarcCote · 2018-08-20T13:04:03Z

doc/examples/viz_timers.py

@@ -0,0 +1,69 @@
+"""
+===============
+Using a timer


Go this warning when running the example.

/home/macote/.local/venvs/dipy/lib/python3.6/site-packages/vtk/util/numpy_support.py:137: FutureWarning: Conversion of the second argument of issubdtype from `complex` to `np.complexfloating` is deprecated. In future, it will be treated as `np.complex128 == np.dtype(complex).type`. assert not numpy.issubdtype(z.dtype, complex), \

Interesting. As far as I know we do not use complex numbers

@skoudoro would be great if we can track down these FutureWarnings appearing when running this example.

MarcCote · 2018-08-20T13:08:59Z

doc/examples/viz_timers.py

+
+showm.start()
+
+window.record(showm.ren, size=(900, 768), out_path="viz_timer.png")


The screenshot recorded says Let's count up to 100 and exit: 101. I would have expected to see 100 instead of 101.

lol okay sure. Will fix that. Sharp eyes @MarcCote!

MarcCote · 2018-08-20T13:25:30Z

dipy/viz/window.py

@@ -591,6 +586,28 @@ def add_window_callback(self, win_callback):
        self.window.AddObserver(vtk.vtkCommand.ModifiedEvent, win_callback)
        self.window.Render()

+    def add_timer_callback(self, repeat, duration, timer_callback):
+        self.iren.AddObserver("TimerEvent", timer_callback)


Rather than using timer_callback directly, we could wrap it with our own method/function that way we can control what arguments will be provided to timer_callback.

For instance, at the moment timer_callback is a function that expects two arguments: a vtkRenderWindowInteractor and a str (the event's name). I doubt common users would use them but they are still forced to provide a callback function that accepts those two arguments. What I propose is the following:

def _wrap_timer_callback(self, timer_callback): def _wrapped_timer_callback(iren, event): timer_callback(self, ...) return _wrapped_timer_callback

then replace line 590 with

self.iren.AddObserver("TimerEvent", self._wrap_timer_callback(timer_callback))

Note ... means we could provide any Python object we deem relevant in addition to the ShowManager instance (i.e. self).

Okay, this is an important point. But does not make much sense changing that in this PR. We need to make decisions about callbacks in a developers meeting. I know that each one of us @karandeepSJ, @dmreagan, @ranveeraggarwal and myself have different suggestions of what will be the optimal strategy with callbacks. My plan is to have that discussion as soon as possible. Possibly after the first split of viz modules. Will send a doodle about that. But for now I suggest moving ahead with this PR. Let me know if you disagree.

MarcCote · 2018-08-27T11:19:47Z

LGTM

Garyfallidis · 2018-09-02T22:45:15Z

@skoudoro this is ready to be merged. Please do merge it. Also, we should check why codecov/patch reports coverage on tests. That looks incorrect. Thanks all for reviewing.

skoudoro · 2018-09-07T15:50:29Z

After some tests, LGTM too. merging

Garyfallidis requested a review from ranveeraggarwal May 20, 2018 14:43

Garyfallidis changed the title ~~Show spheres with different radii size, colors and opacities~~ WIP: Show spheres with different radii size, colors and opacities May 20, 2018

ranveeraggarwal reviewed May 20, 2018

View reviewed changes

This was referenced May 21, 2018

Scipy stopped supporting scipy.misc.imread #1519

Closed

Adds Sphere Object to sphere function in viz.actors Garyfallidis/dipy#56

Open

Garyfallidis mentioned this pull request Jun 1, 2018

Plans for viz module #1544

Closed

11 tasks

Garyfallidis force-pushed the many_spheres branch from 85e360d to ea653be Compare June 3, 2018 00:32

Garyfallidis changed the title ~~WIP: Show spheres with different radii size, colors and opacities~~ Show spheres with different radii size, colors and opacities + add timers Jun 3, 2018

Garyfallidis changed the title ~~Show spheres with different radii size, colors and opacities + add timers~~ Show spheres with different radii, colors and opacities + add timers + add exit a + resolve issue with imread Jun 6, 2018

dmreagan added this to PR needs a review in Viz Module Jun 7, 2018

Garyfallidis added 3 commits August 2, 2018 11:10

NF: visualizing multiple spheres with different centers, colors, radi…

eddf320

…i and opacities

BF: Fixed bugs for providing external sphere

20e9d65

Garyfallidis force-pushed the many_spheres branch from 58316d2 to 20e9d65 Compare August 2, 2018 19:20

Garyfallidis added 2 commits August 9, 2018 15:37

Forcing input data to be contiguous

e51704c

RF: renamed a variable

92db886

Garyfallidis added 2 commits August 9, 2018 16:39

Making this work with old vtk

df7f4fe

Added example in examples_index.rst and valid_examples.txt

a4e43de

MarcCote requested changes Aug 20, 2018

View reviewed changes

Garyfallidis added 3 commits August 26, 2018 18:21

Merge branch 'master' into many_spheres

bf13560

Removed global variables

c0aed55

DOC: explained viz_timers.py tutorial more accurately

4d73163

Garyfallidis added 3 commits August 29, 2018 16:32

Fixed test check and pep8 issue

f5fdd24

Removed timers from list

d769bd4

TEST: added assert for sphere actor

ae369ca

RF: point function is just a simpler version of sphere function

5cb4f8a

skoudoro merged commit 1f0e5bf into dipy:master Sep 7, 2018

Viz Module automation moved this from PR needs a review to Done Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show spheres with different radii, colors and opacities + add timers + add exit a + resolve issue with imread #1528

Show spheres with different radii, colors and opacities + add timers + add exit a + resolve issue with imread #1528

Garyfallidis commented May 20, 2018 •

edited

pep8speaks commented May 20, 2018 •

edited

ranveeraggarwal May 20, 2018

Garyfallidis Jun 3, 2018

ranveeraggarwal commented May 20, 2018

codecov-io commented Aug 9, 2018 •

edited

Garyfallidis commented Aug 18, 2018

MarcCote left a comment

MarcCote Aug 20, 2018

MarcCote Aug 20, 2018

MarcCote Aug 20, 2018

MarcCote Aug 20, 2018

Garyfallidis Aug 26, 2018

MarcCote Aug 27, 2018

MarcCote Aug 20, 2018

Garyfallidis Aug 26, 2018

Garyfallidis Aug 26, 2018

MarcCote Aug 20, 2018

Garyfallidis Aug 26, 2018

MarcCote Aug 27, 2018

MarcCote Aug 20, 2018

Garyfallidis Aug 26, 2018

MarcCote Aug 27, 2018

MarcCote commented Aug 27, 2018

Garyfallidis commented Sep 2, 2018 •

edited

skoudoro commented Sep 7, 2018



		def timer_callback(obj, event):
		global cnt, sphere_actor, showm, tb


		showm.start()

		window.record(showm.ren, size=(900, 768), out_path="viz_timer.png")

Show spheres with different radii, colors and opacities + add timers + add exit a + resolve issue with imread #1528

Show spheres with different radii, colors and opacities + add timers + add exit a + resolve issue with imread #1528

Conversation

Garyfallidis commented May 20, 2018 • edited

pep8speaks commented May 20, 2018 • edited

Comment last updated on September 01, 2018 at 02:13 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranveeraggarwal commented May 20, 2018

codecov-io commented Aug 9, 2018 • edited

Codecov Report

Garyfallidis commented Aug 18, 2018

MarcCote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcCote commented Aug 27, 2018

Garyfallidis commented Sep 2, 2018 • edited

skoudoro commented Sep 7, 2018

Garyfallidis commented May 20, 2018 •

edited

pep8speaks commented May 20, 2018 •

edited

codecov-io commented Aug 9, 2018 •

edited

Garyfallidis commented Sep 2, 2018 •

edited