Skip to content
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

BUG: Fix peaks_slicer actor properties not being enforced. #43

Merged
merged 1 commit into from Jan 9, 2019

Conversation

jhlegarreta
Copy link
Contributor

The actor created in peak_slicer was not having its properties
enforced because the PeakSlicerActor instance was not explicitly
setting its vtk::Property properties. These properties were being set
in the actor of the self.line object, but needed to be set for the
PeakSlicerActor class itself.

Fixes #22.

@dmreagan dmreagan added type:Bug Fix Something isn't working state: needs review labels Jan 7, 2019
@skoudoro skoudoro requested a review from dmreagan January 8, 2019 01:56
@skoudoro skoudoro added this to the v0.2.0 milestone Jan 8, 2019
@jhlegarreta
Copy link
Contributor Author

Same as with #44, Travis errors come from the bz2file module not being found.

@skoudoro
Copy link
Contributor

skoudoro commented Jan 8, 2019

Nibabel team plans to fix this and @dmreagan will review your PR tomorrow so let's wait until Thursday for this PR.

fury/actor.py Outdated Show resolved Hide resolved
The actor created in `peak_slicer` was not having its properties
enforced because the `PeakSlicerActor` instance was not explicitly
setting its `vtk::Property` properties. These properties were being set
in the actor of the `self.line` object, but needed to be set for the
`PeakSlicerActor` class itself.

Fixes fury-gl#22.
@codecov
Copy link

codecov bot commented Jan 9, 2019

Codecov Report

Merging #43 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   81.05%   81.07%   +0.01%     
==========================================
  Files          13       13              
  Lines        3062     3065       +3     
  Branches      349      349              
==========================================
+ Hits         2482     2485       +3     
  Misses        476      476              
  Partials      104      104
Impacted Files Coverage Δ
fury/actor.py 90.54% <100%> (+0.05%) ⬆️

@dmreagan dmreagan merged commit 97d8f31 into fury-gl:master Jan 9, 2019
@jhlegarreta
Copy link
Contributor Author

@dmreagan May be I should have also modified the test and add some opacity values to less than 1.0 to actually proof that the patch is working?

@jhlegarreta jhlegarreta deleted the FixPeakSlicerActorProperties branch January 9, 2019 23:08
@dmreagan
Copy link
Contributor

dmreagan commented Jan 9, 2019

More tests, or more explicit tests, are usually a good idea. If you have time, go for it. But since this PR fixes an existing feature rather than adding something new, I'm not going to be strict about it.

@jhlegarreta
Copy link
Contributor Author

@dmreagan looks reasonable. Thanks. Proposed PR #48 to effectively exercise the property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants