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

A lightweight UI for medical visualizations #1: Button and Panel #1140

Merged
merged 28 commits into from Feb 26, 2017

Conversation

Projects
None yet
7 participants
@ranveeraggarwal
Member

ranveeraggarwal commented Nov 2, 2016

This is one of the several PRs that will be made over time to incorporate features of #1111 with tests.

.gitignore Outdated
@@ -30,3 +30,4 @@ __config__.py
.coverage
.buildbot.patch
.eggs/
.idea/

This comment has been minimized.

@MarcCote

MarcCote Nov 2, 2016

Contributor

I suggest you remove this changes and use a local .gitignore_global instead. See https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Nov 2, 2016

Member

Thanks for the suggestion 😄
I have removed that commit.

@ranveeraggarwal ranveeraggarwal force-pushed the ranveeraggarwal:ui branch from 04cba17 to b923d5b Nov 2, 2016

@codecov-io

This comment has been minimized.

codecov-io commented Nov 2, 2016

Codecov Report

Merging #1140 into master will increase coverage by 0.12%.
The diff coverage is 92.1%.

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
+ Coverage    85.8%   85.93%   +0.12%     
==========================================
  Files         215      219       +4     
  Lines       26001    26383     +382     
  Branches     2668     2706      +38     
==========================================
+ Hits        22310    22671     +361     
- Misses       3038     3052      +14     
- Partials      653      660       +7
Impacted Files Coverage Δ
dipy/viz/interactor.py 97.4% <100%> (+1.29%)
dipy/viz/window.py 61.14% <100%> (+2.3%)
dipy/viz/tests/test_ui.py 91.02% <91.02%> (ø)
dipy/viz/ui.py 92.15% <92.15%> (ø)
dipy/data/init.py 89.33% <ø> (ø)
dipy/workflows/tests/test_masking.py 91.66% <ø> (ø)
dipy/workflows/mask.py 95% <ø> (ø)
... and 2 more

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 94a4668...796b6ce. Read the comment docs.

@ranveeraggarwal ranveeraggarwal force-pushed the ranveeraggarwal:ui branch from 8588b20 to 7c2d91d Nov 3, 2016

ranveeraggarwal added some commits Nov 3, 2016

pass
broken_ui = BrokenUI()
npt.assert_raises(NotImplementedError, broken_ui.get_actors())

This comment has been minimized.

@MarcCote

MarcCote Nov 4, 2016

Contributor

You need to let npt.assert_raises call the method like so: npt.assert_raises(NotImplementedError, broken_ui.get_actors)

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Nov 4, 2016

Member

This doesn't raise an error for me. Is it because I actually implemented a

def foo(self):
     pass

for all the unimplemented functions? If I don't they throw an error during initialisation. How do I prevent that?

broken_ui = BrokenUI()
npt.assert_raises(NotImplementedError, broken_ui.get_actors())
npt.assert_raises(NotImplementedError, broken_ui.set_center((1, 2)))

This comment has been minimized.

@MarcCote

MarcCote Nov 4, 2016

Contributor

You need to let npt.assert_raises call the method like so: npt.assert_raises(NotImplementedError, broken_ui.set_center, (1, 2))

iren = ren.GetRenderWindow().GetInteractor().GetInteractorStyle()
for callback in self._callbacks:
if not isinstance(iren, CustomInteractorStyle):

This comment has been minimized.

@MarcCote

MarcCote Nov 4, 2016

Contributor

To test this, you'll need to a ShowManger object that doesn't use the CustomInteractorStyle. For example: showm = ShowManager(..., interactor_style='trackball'), then adding a UI component to the renderer would raise the exception.

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Have you tested this?

This comment has been minimized.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Nov 4, 2016

@MarcCote @Garyfallidis I have modified the button element to use polygons as suggested by Marc. Is there anything else to be done for this PR?

@ranveeraggarwal ranveeraggarwal changed the title from WIP: A lightweight UI for medical visualizations to A lightweight UI for medical visualizations #1: Button Nov 4, 2016

@skoudoro

Hi @ranveeraggarwal , everything works like a charm on my environment. I just add a small comment for checking icon extension.

tc.InsertComponent(0,0, 0.0); tc.InsertComponent(0,1, 0.0)
tc.InsertComponent(1,0, 1.0); tc.InsertComponent(1,1, 0.0)
tc.InsertComponent(2,0, 1.0); tc.InsertComponent(2,1, 1.0)
tc.InsertComponent(3,0, 0.0); tc.InsertComponent(3,1, 1.0)

This comment has been minimized.

@skoudoro

skoudoro Dec 15, 2016

Member

PEP 8: missing white space after ','

tc.SetNumberOfTuples(4)
tc.InsertComponent(0,0, 0.0); tc.InsertComponent(0,1, 0.0)
tc.InsertComponent(1,0, 1.0); tc.InsertComponent(1,1, 0.0)
tc.InsertComponent(2,0, 1.0); tc.InsertComponent(2,1, 1.0)

This comment has been minimized.

@skoudoro

skoudoro Dec 15, 2016

Member

PEP 8: missing white space after ','

tc.SetNumberOfComponents(2)
tc.SetNumberOfTuples(4)
tc.InsertComponent(0,0, 0.0); tc.InsertComponent(0,1, 0.0)
tc.InsertComponent(1,0, 1.0); tc.InsertComponent(1,1, 0.0)

This comment has been minimized.

@skoudoro

skoudoro Dec 15, 2016

Member

PEP 8: missing white space after ','

tc = vtk.vtkFloatArray()
tc.SetNumberOfComponents(2)
tc.SetNumberOfTuples(4)
tc.InsertComponent(0,0, 0.0); tc.InsertComponent(0,1, 0.0)

This comment has been minimized.

@skoudoro

skoudoro Dec 15, 2016

Member

PEP 8: missing white space after ','

icons = {}
for icon_name, icon_fname in icon_fnames.items():
png = vtk.vtkPNGReader()
png.SetFileName(icon_fname)

This comment has been minimized.

@skoudoro

skoudoro Dec 15, 2016

Member

You should check icon filename extension, then ignore and warn the users when it is the wrong extension.
Some people like svg format for icon and vtk generate a segmentation fault in this case.

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Dec 16, 2016

@skoudoro thank you for your suggestions. @MarcCote and I are currently working on making the API even more simple than what we currently have. I will be incorporating the changes you've suggested in the upcoming commits to this Pull Request.
Also, glad to know you're interested in this sub-project. Since we're in the beginning phase (of the visualization module, that is), we could use some suggestions 😄

@coveralls

This comment has been minimized.

coveralls commented Jan 21, 2017

Coverage Status

Coverage increased (+0.4%) to 88.413% when pulling 80ea1f7 on ranveeraggarwal:ui into e8eeb70 on nipy:master.

@MarcCote

First few comments. A lot of docstrings are incomplete. The code is running fine on my machine, this is awesome :D

While adding UI elements to the renderer, we need to go over all the
sub-elements that come with it and add those to the renderer too.
There are several features that are common to all the UI elements:
- ui_param : This is an attribute that can be passed to the UI object

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

@Garyfallidis @arokem what docstring structure should be used here? I think we should follow a similar pattern as docstring for function. For instance

Attributes
----------
ui_param : object
  Description...

This comment has been minimized.

@arokem

arokem Jan 24, 2017

Member

Agreed. @ranveeraggarwal : Please take a look at the numpy docstring standard: https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt

sub-elements that come with it and add those to the renderer too.
There are several features that are common to all the UI elements:
- ui_param : This is an attribute that can be passed to the UI object
by the interactor. Thanks to Python's dynamic type-setting

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Remove "Thanks to Python's dynamic type-setting..." since the attribute type is going to be mentioned in the dosctring anyway.

- ui_param : This is an attribute that can be passed to the UI object
by the interactor. Thanks to Python's dynamic type-setting
this parameter can be anything.
- ui_list : This is used when there are more than one UI elements inside

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Maybe just mentioned: "List of all UI elements contained in this UI element. These subcomponents will be added automatically to the renderer at the same as this one." or something like that.

this parameter can be anything.
- ui_list : This is used when there are more than one UI elements inside
a UI element. Inside the renderer, they're all iterated and added.
- parent_UI: This is useful of there is a parent UI element and its reference

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Start by saying what it is, then tell how it can be useful.

iren = ren.GetRenderWindow().GetInteractor().GetInteractorStyle()
for callback in self._callbacks:
if not isinstance(iren, CustomInteractorStyle):

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Have you tested this?

def add_element(self, element, position_type, position):
""" Adds an elements to the panel.
The center of the rectangular panel is its bottom lower position.

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Newline before the body.

elif self.alignment == "right":
self.set_center((self.center[0] + window_size_change[0], self.center[1] + window_size_change[1]))
else:
pass

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Newline at the end

elif self.alignment == "right":
self.set_center((self.center[0] + window_size_change[0], self.center[1] + window_size_change[1]))
else:
pass

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Should raise an ValueError if self.alignment is wrong.

self.right_button_state = "dragging"
self.on_right_mouse_button_drag(i_ren, obj, self)
else:
self.on_mouse_hover(i_ren, obj, self)

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Unless you specifically add a prop to the list of active_props in the interactor (https://github.com/nipy/dipy/blob/master/dipy/viz/interactor.py#L76), the MouseMove callback are only called when one of the mouse buttons is held down (it actually works in your viz_ui.py example if you middle click the button, hold it and move the mouse). See https://github.com/nipy/dipy/blob/master/dipy/viz/interactor.py#L153

i_ren.event.abort()
@staticmethod
def left_button_release_callback(i_ren, obj, self):

This comment has been minimized.

@MarcCote

MarcCote Jan 24, 2017

Contributor

Can you explain why these callback methods are static? And why they receive a self as the last parameter? Couldn't we just make them methods?

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jan 24, 2017

Member

Isn't that how all callbacks are to be defined for VTK?

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

Oh! I see, it is because of how we actually called the callback functions (https://github.com/nipy/dipy/pull/1140/files#diff-a1072548746ff0b78f876c2efab5bd0eR286).

We should use self as an argument name since it has a special meaning. Let's call it ui_object.

@ranveeraggarwal ranveeraggarwal force-pushed the ranveeraggarwal:ui branch from c5e118b to fb24eb2 Jan 24, 2017

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Jan 25, 2017

@MarcCote I have addressed most of the comments through the last two commits. Anything left?

@@ -129,3 +150,11 @@ def modify_button_callback(i_ren, obj, button):
for event, count in expected:
npt.assert_equal(states[event], count, err_msg=msg.format(event))
# Dummy Show Manager

This comment has been minimized.

@MarcCote

MarcCote Feb 9, 2017

Contributor

Indentation

@MarcCote

Almost there. Mostly dosctring and PEP8 stuff. I'll have to review the the panel machinery tomorrow.

pass
def add_callback(self, event_type, callback):
""" Adds events to an actor.
Parameters

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

newline

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

Actually, probably we don't need a docstring for this function, since it is only used for testing. @Garyfallidis what do you think?

# Rectangle
rectangle_test = ui.Rectangle2D(size=(10, 10))
rectangle_test.get_actors()
another_rectangle_test = ui.Rectangle2D(size=(1,1))

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

PEP8: Space after comma

a UI element. Inside the renderer, they're all iterated and added.
- parent_UI: This is useful of there is a parent UI element and its reference
needs to be passed down to the child.
While adding UI elements to the renderer, we go over all the sub-elements

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

Newline after the short description.

# Fill the placeholder with the command ID returned by VTK.
cmd_id[0] = prop.AddObserver(event_type, _callback, priority)
cmd_id[0] = prop.AddObserver(event_type, _callback, priority)

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

PEP8: Make sure there is a newline a the end of the file.

This comment has been minimized.

@ranveeraggarwal
class UI(object):
""" An umbrella class for all UI elements.
While adding UI elements to the renderer, we go over all the sub-elements

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

PEP8: Newline after a short description.

def set_center(self, position):
""" Sets the panel center to position.
The center of the rectangular panel is its bottom lower position.

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

newline

ui_element[0].set_center((ui_element[2], ui_element[3]))
@staticmethod
def left_button_press(i_ren, obj, element):

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

element here is expected to be a Panel2D object? Maybe change the name to something more precise like panel2d_object.

i_ren.force_render()
def re_align(self, window_size_change):
""" Re-organises the elements in case the

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

PEP8: The short description should fit on a single line.

Parameters
----------
window_size_change : (int, int)

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

description

@@ -231,6 +233,10 @@ def renderer(background=None):
>>> fvtk.add(r,c)
>>> #fvtk.show(r)
"""
deprecation_msg = "Method 'dipy.viz.window.renderer' is deprecated, instead use class 'dipy.viz.window.Renderer'."

This comment has been minimized.

@MarcCote

MarcCote Feb 10, 2017

Contributor

Line too long. Do something like:

deprecation_msg = ("Method 'dipy.viz.window.renderer' is deprecated, instead" 
                   " use class 'dipy.viz.window.Renderer'.")
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 18, 2017

Hi @ranveeraggarwal and @MarcCote. I generally like the design. I think it is a good balance between what I was thinking and what Marco was thinking about the UI.

I did try the code and worked for me. However, you still have many pep8 issues in ui.py (mostly regarding line length). Please do correct those.

Now in test_ui.py if I change the parameter to recording=True I get this warning after closing the window

Generic Warning: In /build/vtk6-YpT4yb/vtk6-6.2.0+dfsg1/Common/Core/vtkObject.cxx, line 533
Passive observer should not call AddObserver or RemoveObserver in callback.

Which is something not critical but it is good to know that it does exist.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 18, 2017

@ranveeraggarwal do fix the last pep8 issues and then I will merge. After that we can move to the other UI components (slider, 3d slider and filedialogue).

@coveralls

This comment has been minimized.

coveralls commented Feb 22, 2017

Coverage Status

Coverage increased (+0.1%) to 88.422% when pulling e8df6f0 on ranveeraggarwal:ui into 94a4668 on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 26, 2017

All good from my end.

@coveralls

This comment has been minimized.

coveralls commented Feb 26, 2017

Coverage Status

Coverage increased (+0.1%) to 88.422% when pulling 796b6ce on ranveeraggarwal:ui into 94a4668 on nipy:master.

@Garyfallidis Garyfallidis merged commit 32ed56b into nipy:master Feb 26, 2017

4 checks passed

codecov/patch 92.1% of diff hit (target 85.8%)
Details
codecov/project 85.93% (+0.12%) compared to 94a4668
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 88.422%
Details

@ranveeraggarwal ranveeraggarwal changed the title from A lightweight UI for medical visualizations #1: Button to A lightweight UI for medical visualizations #1: Button and Panel Aug 8, 2017

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1140 from ranveeraggarwal/ui
A lightweight UI for medical visualizations nipy#1: Button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment