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

Better fvtk.viz error when no VTK installed #1243

Merged
merged 2 commits into from Jun 20, 2017

Conversation

Projects
None yet
4 participants
@ghoshbishakh
Member

ghoshbishakh commented May 14, 2017

fixes #1238

@coveralls

This comment has been minimized.

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.004%) to 85.603% when pulling 9e4a853 on ghoshbishakh:betterErrors into ceed367 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.004%) to 85.603% when pulling 9e4a853 on ghoshbishakh:betterErrors into ceed367 on nipy:master.


def __call__(self, *args, **kwargs):
''' Raise informative error while calling '''
raise TripWireError(self._msg)

This comment has been minimized.

@arokem

arokem May 15, 2017

Member

Could you please write a test that exercises this code?

@codecov-io

This comment has been minimized.

codecov-io commented May 16, 2017

Codecov Report

Merging #1243 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
+ Coverage   87.04%   87.05%   +<.01%     
==========================================
  Files         225      225              
  Lines       28109    28113       +4     
  Branches     3014     3014              
==========================================
+ Hits        24468    24473       +5     
  Misses       2968     2968              
+ Partials      673      672       -1
Impacted Files Coverage Δ
dipy/utils/tripwire.py 100% <100%> (ø) ⬆️
dipy/viz/fvtk.py 62.41% <100%> (+0.08%) ⬆️
dipy/utils/tests/test_tripwire.py 93.33% <100%> (+0.47%) ⬆️
dipy/utils/optpkg.py 71.42% <0%> (+3.57%) ⬆️

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 ceed367...f5457c4. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented May 16, 2017

Coverage Status

Coverage increased (+0.0003%) to 85.607% when pulling f5457c4 on ghoshbishakh:betterErrors into ceed367 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented May 16, 2017

Coverage Status

Coverage increased (+0.0003%) to 85.607% when pulling f5457c4 on ghoshbishakh:betterErrors into ceed367 on nipy:master.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented May 16, 2017

@arokem done!

@@ -79,6 +79,9 @@
msg = "Python VTK is not installed"
warn(msg)

ren, have_ren, _ = optional_package('dipy.viz.window.ren',

This comment has been minimized.

@arokem

arokem May 18, 2017

Member

Reading this again -- do you understand why the line above this one throws a warning, instead of raising an error?

This comment has been minimized.

@ghoshbishakh

ghoshbishakh May 19, 2017

Member

@arokem

ren, have_ren, _ = optional_package('dipy.viz.window.ren',
                                        'Python VTK is not installed')

returns a TripWire object into ren

So calling ren will only raise an exception, not the line 82.

This comment has been minimized.

@arokem

arokem May 19, 2017

Member

I mean: if we don't have VTK installed, why don't we just raise an exception in the previous line (line 80)? Why do we bother going ahead and trying to do anything?

This comment has been minimized.

@ghoshbishakh

ghoshbishakh May 20, 2017

Member

@arokem I am not sure about that design decision. But there must be a reason for example it says # Allow import, but disable doctests if we don't have vtk, although I am not sure what that means. @Garyfallidis

This comment has been minimized.

@arokem

arokem May 20, 2017

Member

@Garyfallidis - I don't understand this. Is there any reason line 80 doesn't raise an error? If have_vtk is False, we're bound to hit an error sooner or later, no?

@arokem

This comment has been minimized.

Member

arokem commented Jun 20, 2017

Merging. Will follow up with another PR if needed.

@arokem arokem merged commit 8c32128 into nipy:master Jun 20, 2017

4 checks passed

codecov/patch 100% of diff hit (target 87.04%)
Details
codecov/project 87.05% (+<.01%) compared to ceed367
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0003%) to 85.607%
Details

arokem added a commit to arokem/dipy that referenced this pull request Jun 20, 2017

Garyfallidis added a commit that referenced this pull request Jun 21, 2017

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

Merge pull request nipy#1243 from ghoshbishakh/betterErrors
Better fvtk.viz error when no VTK installed

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment