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 #5: 2D Circular Slider #1222
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1222 +/- ##
=========================================
+ Coverage 85.92% 86.1% +0.18%
=========================================
Files 221 223 +2
Lines 27270 27893 +623
Branches 2785 2828 +43
=========================================
+ Hits 23431 24018 +587
- Misses 3154 3177 +23
- Partials 685 698 +13
Continue to review full report at Codecov.
|
Hi @MarcCote can you check this PR and see if it can be approved? In the meantime, Ranveer make sure you have no identation (or PEP 8) issues here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I started to make my review and decided to do some refactoring first, so DiskSlider2D
ressembles more LineSlider2D
.
dipy/viz/ui.py
Outdated
self.probe.SetPosition(( | ||
self.base_disk_center[0] + self.base_disk_radius, | ||
self.base_disk_center[1])) | ||
# /Probe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the /...
ending comment.
dipy/viz/ui.py
Outdated
float(dy) / float(dx)) * float(x2 - center[0]) | ||
|
||
d1 = (x1 - point[0])*(x1 - point[0]) + (y1 - point[1])*(y1 - point[1]) | ||
d2 = (x2 - point[0])*(x2 - point[0]) + (y2 - point[1])*(y2 - point[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8: Spaces around *
dipy/viz/ui.py
Outdated
return angle | ||
|
||
def move_probe(self, click_position): | ||
"""Moves the probe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8: Space after """
dipy/viz/ui.py
Outdated
|
||
Parameters | ||
---------- | ||
click_position: (float, float) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8: make sure you put a space between the parameter name and:
dipy/viz/ui.py
Outdated
return x2, y2 | ||
|
||
def get_angle(self, coordinates): | ||
""" Gets the angle made with the X-Axis for calculating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8: The short description should fit on one line. What about:
""" Gets the angle the cursor makes with the X-Axis
The angle varies between 0-360.
"""
dipy/viz/ui.py
Outdated
perpendicular = -center[1] + coordinates[1] | ||
base = -center[0] + coordinates[0] | ||
|
||
angle = math.degrees(math.atan2(float(perpendicular), float(base))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use numpy
instead of the math
library.
angle = np.rad2deg(np.arctan2(perpendicular, base))
dipy/viz/ui.py
Outdated
Position of the system. | ||
base_disk_radius: float | ||
Average radius of the base disk. | ||
probe_outer_radius: int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use "handle" instead of "probe". What do you think?
@MarcCote looks good, I just made the example clearer and initialised the disk actors to None (easier to understand, IMO). We can merge this once the tests are complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @Garyfallidis you can have a look.
Cool, thanks @ranveeraggarwal. Let's move to the next PRs. :) |
A lightweight UI for medical visualizations dipy#5: 2D Circular Slider
Continuing from #1205 and the fourth installment of #1111, this PR adds a Circular Slider element to the Viz module.
Complete blogpost here: http://ranveeraggarwal.com/blog/gsoc-2016-summary
To Do