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

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

Merged
merged 14 commits into from Sep 7, 2018

Conversation

Projects
6 participants
@Garyfallidis
Copy link
Member

Garyfallidis commented May 20, 2018

  • 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.

@Garyfallidis Garyfallidis requested a review from ranveeraggarwal May 20, 2018

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented May 20, 2018

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

@Garyfallidis 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


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

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal May 20, 2018

Member

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

This comment has been minimized.

@Garyfallidis

Garyfallidis Jun 3, 2018

Member

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

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented May 20, 2018

Looks good! A couple of points:

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

@Garyfallidis Garyfallidis referenced this pull request Jun 1, 2018

Closed

Plans for viz module #1544

2 of 11 tasks complete

@Garyfallidis Garyfallidis force-pushed the Garyfallidis:many_spheres branch from 85e360d to ea653be Jun 3, 2018

@Garyfallidis 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 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 dmreagan added this to PR needs a review in Viz Module Jun 7, 2018

Garyfallidis added some commits May 14, 2018

NF: added timer callback in ShowManager
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

@Garyfallidis Garyfallidis force-pushed the Garyfallidis:many_spheres branch from 58316d2 to 20e9d65 Aug 2, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 9, 2018

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link
Member

Garyfallidis commented Aug 18, 2018

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

@MarcCote
Copy link
Contributor

MarcCote left a comment

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

showm.initialize()

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

This comment has been minimized.

@MarcCote

MarcCote Aug 20, 2018

Contributor

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


showm.initialize()

def timer_callback(obj, event):

This comment has been minimized.

@MarcCote

MarcCote Aug 20, 2018

Contributor

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



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

This comment has been minimized.

@MarcCote

MarcCote Aug 20, 2018

Contributor

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).

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

showm.start()

This comment has been minimized.

@MarcCote

MarcCote Aug 20, 2018

Contributor

This should be commented?

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 26, 2018

Member

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? :)

This comment has been minimized.

@MarcCote

MarcCote Aug 27, 2018

Contributor

Oh nice! ☺️

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

This comment has been minimized.

@MarcCote

MarcCote Aug 20, 2018

Contributor

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), \

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 26, 2018

Member

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

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 26, 2018

Member

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


showm.start()

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

This comment has been minimized.

@MarcCote

MarcCote Aug 20, 2018

Contributor

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

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 26, 2018

Member

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

This comment has been minimized.

@MarcCote

MarcCote Aug 27, 2018

Contributor

;)

@@ -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)

This comment has been minimized.

@MarcCote

MarcCote Aug 20, 2018

Contributor

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).

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 26, 2018

Member

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.

This comment has been minimized.

@MarcCote

MarcCote Aug 27, 2018

Contributor

I agree.

Garyfallidis added some commits Aug 26, 2018

@MarcCote

This comment has been minimized.

Copy link
Contributor

MarcCote commented Aug 27, 2018

LGTM

Garyfallidis added some commits Aug 29, 2018

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Sep 2, 2018

@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

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 7, 2018

After some tests, LGTM too. merging

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

2 of 3 checks passed

codecov/patch 70.87% of diff hit (target 87.33%)
Details
codecov/project 87.34% (+0.01%) compared to a1e1f2e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

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