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

Fix for "DiskSlider does not rotate actor in opposite direction" #1460

Merged
merged 3 commits into from Mar 22, 2018

Conversation

Projects
4 participants
@karandeepSJ
Contributor

karandeepSJ commented Mar 13, 2018

This PR fixes issue #1458

  • The problem was that the actor was being rotated every-time by the angle between the line joining center of base_disk and handle and the horizontal axis. It should have been rotated by angle equal to the change in angle between the line and horizontal axis.
  • To implement this, an attribute - pvalue was added to the DiskSlider2D class to store the previous value of rotation. This is used to calculate the angle by which the actor must be rotated.
@codecov-io

This comment has been minimized.

codecov-io commented Mar 14, 2018

Codecov Report

Merging #1460 into master will increase coverage by 0.01%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1460      +/-   ##
=========================================
+ Coverage   87.49%   87.5%   +0.01%     
=========================================
  Files         241     241              
  Lines       30789   30799      +10     
  Branches     3322    3322              
=========================================
+ Hits        26939   26952      +13     
+ Misses       3074    3072       -2     
+ Partials      776     775       -1
Impacted Files Coverage Δ
dipy/viz/ui.py 89.3% <90%> (ø) ⬆️
dipy/reconst/forecast.py 92.22% <0%> (+2.07%) ⬆️

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 f6af802...a3c654d. Read the comment docs.

@dmreagan dmreagan added this to UI PR needs a review in Viz Module Mar 14, 2018

@dmreagan dmreagan requested review from dmreagan and ranveeraggarwal Mar 14, 2018

@ranveeraggarwal

Great job finding this bug! A couple of small fixes and you should be good to go.

@@ -1921,7 +1921,7 @@ def __init__(self, position=(0, 0),
self.center = np.array(position)
self.min_value = min_value
self.max_value = max_value
self.initial_value = initial_value

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 15, 2018

Member

This needs to be added to the class docstring.

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 17, 2018

Member

@karandeepSJ please fix this too.

@@ -1939,6 +1939,7 @@ def __init__(self, position=(0, 0),
# By setting the value, it also updates everything.
self.value = initial_value
self.pvalue = initial_value

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 15, 2018

Member

This too needs to be added to the docstring.
Also, IMO an explicit previous_value would be a better name for this attribute.

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:rotate-actor-fix branch from cca372f to 4351c5b Mar 15, 2018

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Mar 19, 2018

This looks good to me. @skoudoro @dmreagan I think this is ready for merge.

@dmreagan

Looks good. I'll merge it.

@dmreagan dmreagan merged commit b8e01ee into nipy:master Mar 22, 2018

3 checks passed

codecov/patch 90% of diff hit (target 87.49%)
Details
codecov/project 87.5% (+0.01%) compared to f6af802
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Viz Module automation moved this from UI PR needs a review to Done Mar 22, 2018

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

Merge pull request nipy#1460 from karandeepSJ/rotate-actor-fix
Fix for "DiskSlider does not rotate actor in opposite direction"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment