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

PyTables Error-handling #1307

Merged
merged 6 commits into from Jul 30, 2017

Conversation

Projects
None yet
5 participants
@skoudoro
Member

skoudoro commented Jul 24, 2017

The goal of this PR is to resolve #1306.

@arokem @aarya22, can you have a look ?

Thanks !

@aarya22

This comment has been minimized.

Contributor

aarya22 commented Jul 24, 2017

Nice job on fixing the NameError.

@arokem

LGTM overall. A couple of small questions regarding the error that gets raised.

odf = None
affine = pam.affine if hasattr(pam, 'affine') else affine
shm_coeff = pam.shm_coeff if hasattr(pam, 'shm_coeff') else None
odf = pam.odf if hasattr(pam, 'odf') else None

This comment has been minimized.

@arokem

arokem Jul 24, 2017

Member

Elegant

This comment has been minimized.

@skoudoro

skoudoro Jul 24, 2017

Member

Thanks ;-)

pam.odf = np.zeros((10, 10, 10, sphere.vertices.shape[0]))
if not have_tables:
npt.assert_raises(TripWireError, save_peaks, fname, pam)

This comment has been minimized.

@arokem

arokem Jul 24, 2017

Member

Looks like this might raise an ImportError?

This comment has been minimized.

@skoudoro

skoudoro Jul 24, 2017

Member

save_peaks raise TripWireError , however _safe_save raise ImportError, so it is ok here. To be more consistent, I generate a TripWireError via a fake call in _safe_save now.

This comment has been minimized.

@arokem

arokem Jul 24, 2017

Member

Is the error handling in _safe_save tested elsewhere? Would you mind writing a test, if it isn't?

This comment has been minimized.

@skoudoro

skoudoro Jul 25, 2017

Member

👍 test added

else:
func_create_carray = f.create_carray
if not have_tables:
raise ImportError('PyTables is not installed')

This comment has been minimized.

@arokem

arokem Jul 24, 2017

Member

Is there some way to get the TripWireError here (instead of an ImportError?)

@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage decreased (-0.01%) to 85.425% when pulling d3d017b on skoudoro:issue_1306_pytables into 08d8d90 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage decreased (-0.01%) to 85.425% when pulling d3d017b on skoudoro:issue_1306_pytables into 08d8d90 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 24, 2017

Codecov Report

Merging #1307 into master will decrease coverage by 0.03%.
The diff coverage is 75.51%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1307      +/-   ##
=========================================
- Coverage   87.13%   87.1%   -0.04%     
=========================================
  Files         228     228              
  Lines       28800   28816      +16     
  Branches     3093    3092       -1     
=========================================
+ Hits        25094   25099       +5     
- Misses       3003    3014      +11     
  Partials      703     703
Impacted Files Coverage Δ
dipy/io/dpy.py 94.02% <100%> (+1.38%) ⬆️
dipy/io/tests/test_io_peaks.py 89.81% <73.68%> (-7.66%) ⬇️
dipy/io/peaks.py 89.79% <80%> (-2.66%) ⬇️

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 08d8d90...2b0af87. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage decreased (-0.01%) to 85.425% when pulling d3d017b on skoudoro:issue_1306_pytables into 08d8d90 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage decreased (-0.01%) to 85.425% when pulling d3d017b on skoudoro:issue_1306_pytables into 08d8d90 on nipy:master.

@arokem

This is converging. Just one more question about the test.

func_create_carray = f.create_carray
if not have_tables:
# We generate a TripWireError via this call
_ = tables.any_attributes

This comment has been minimized.

@arokem

arokem Jul 24, 2017

Member

👍

This seems fine to me.

pam.odf = np.zeros((10, 10, 10, sphere.vertices.shape[0]))
if not have_tables:
npt.assert_raises(TripWireError, save_peaks, fname, pam)

This comment has been minimized.

@arokem

arokem Jul 24, 2017

Member

Is the error handling in _safe_save tested elsewhere? Would you mind writing a test, if it isn't?

@coveralls

This comment has been minimized.

coveralls commented Jul 25, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.405% when pulling 2b0af87 on skoudoro:issue_1306_pytables into 08d8d90 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jul 25, 2017

+1 for the merge from me. Anyone else want to take a look?

If not, I will merge in a couple of days.

@arokem arokem merged commit 0c10a43 into nipy:master Jul 30, 2017

1 of 4 checks passed

codecov/patch 75.51% of diff hit (target 87.13%)
Details
codecov/project 87.1% (-0.04%) compared to 08d8d90
Details
coverage/coveralls Coverage decreased (-0.03%) to 85.405%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:issue_1306_pytables branch Jul 31, 2017

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