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

actor.slicer.copy() copies opacity set via actor.slicer.opacity() #1452

Merged
merged 3 commits into from Mar 22, 2018

Conversation

Projects
5 participants
@karandeepSJ
Contributor

karandeepSJ commented Mar 9, 2018

This PR fixes #1438
Changes:

  • In copy function of actor.py, im_actor.opacity(opacity) changed to im_actor.opacity(self.GetOpacity()).
  • viz-advanced.py no longer needs to use image_actor_x.opacity(slicer_opacity) to copy opacity of image_actor_z to image_actor_x
  • Tests added.

@karandeepSJ karandeepSJ changed the title from actor.slicer.copy() copies opacity set via actor.slicer.opacity() #1438 to actor.slicer.copy() copies opacity set via actor.slicer.opacity() Mar 9, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Mar 9, 2018

Codecov Report

Merging #1452 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1452      +/-   ##
==========================================
- Coverage   87.42%   87.41%   -0.02%     
==========================================
  Files         239      239              
  Lines       30585    30591       +6     
  Branches     3291     3291              
==========================================
+ Hits        26740    26741       +1     
- Misses       3076     3082       +6     
+ Partials      769      768       -1
Impacted Files Coverage Δ
dipy/viz/actor.py 83.01% <0%> (ø) ⬆️
dipy/viz/tests/test_actors.py 73.67% <0%> (-0.96%) ⬇️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

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 18f3d00...199bde1. Read the comment docs.

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

slicer_lut2 = slicer_lut.copy()
npt.assert_equal(slicer_lut2.GetOpacity(),0.5);

This comment has been minimized.

@dmreagan

dmreagan Mar 9, 2018

Contributor

No semicolon. Also whitespace after the comma. Consider using a pep8 linter with your editor to catch little things like these.

This comment has been minimized.

@karandeepSJ

karandeepSJ Mar 9, 2018

Contributor

done

@@ -175,7 +175,7 @@ def copy(self):
im_actor = ImageActor()
im_actor.input_connection(self.output)
im_actor.SetDisplayExtent(*self.GetDisplayExtent())
im_actor.opacity(opacity)
im_actor.opacity(self.GetOpacity())
im_actor.tolerance(picking_tol)

This comment has been minimized.

@dmreagan

dmreagan Mar 9, 2018

Contributor

While you're at it, can you also fix this same issue for the tolerance?

This comment has been minimized.

@karandeepSJ

karandeepSJ Mar 9, 2018

Contributor

done

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

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Mar 9, 2018

These updates look good to me. Nice work, @karandeepSJ . I had a couple test failures this time, but I think they might be on my end. I'll have to look into it later. Let's see what Travis says.

@ranveeraggarwal

Minor nitpick, but otherwise looks good.
I am unable to test this on my machine right now, I'll get back when I have the build ready.

npt.assert_equal(slicer_lut2.GetOpacity(), 0.5)
npt.assert_equal(slicer_lut2.picker.GetTolerance(), 0.03)
slicer_lut.opacity(1)
slicer_lut.tolerance(0.025)

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 10, 2018

Member

Correct me if I am wrong, but aren't lines 114-117 redundant? @dmreagan
116-117 can still kind of make sense, but the properties set in 114-115 aren't being used/asserted anywhere.

This comment has been minimized.

@karandeepSJ

karandeepSJ Mar 11, 2018

Contributor

Done

slicer_lut.display(10, None, None)
slicer_lut.display(None, 10, None)
slicer_lut.display(None, None, 10)
slicer_lut.opacity(0.5)
slicer_lut.tolerance(0.03)

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 10, 2018

Member

Since we're explicitly testing copy starting next line, it's preferable to add a newline after this.

This comment has been minimized.

@karandeepSJ

karandeepSJ Mar 11, 2018

Contributor

Done

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Mar 14, 2018

I think this is ready to go. @skoudoro any reason to worry about the codecov?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 14, 2018

No reason to be worried @dmreagan. test_slicer is skipped for vtk5 and Travis run all tests with vtk5. Can you just run the tests on your machine and be sure that everything is ok. Then, you can merge it

@ranveeraggarwal

Everything looks good to me.

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

1 of 3 checks passed

codecov/patch 0% of diff hit (target 87.42%)
Details
codecov/project 87.41% (-0.02%) compared to 18f3d00
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

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Mar 22, 2018

Thanks for the PR, @karandeepSJ!

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

Merge pull request nipy#1452 from karandeepSJ/bug-1438-fix
actor.slicer.copy() copies opacity set via actor.slicer.opacity()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment