detecting if the input is empty early on! #611

raamana · 2018-03-19T04:23:12Z

Apparently no one needed it so far ;)

Not sure where to write tests for it, so wanna check with you guys before taking appropriate action.

coveralls · 2018-03-19T04:50:58Z

Coverage increased (+0.002%) to 93.962% when pulling a974b70 on raamana:empty_files into 1140e18 on nipy:master.

effigies

If I understand the paths here correctly, we should get fall-through behavior here and you should be seeing an ImageFileError with message "Cannot work out file type ...".

Is this what you're seeing? I agree that it could be more informative. (If it's not, could you copy in what you are seeing?)

effigies · 2018-03-19T19:11:28Z

nibabel/loadsave.py

+
+    if op.getsize(filename) <= 0:
+        raise FileEmptyError("Given file is empty: '%s'" % filename)
+


PEP8 is complaining about whitespace on this line.

effigies · 2018-03-19T19:14:09Z

nibabel/loadsave.py

@@ -38,6 +38,10 @@ def load(filename, **kwargs):
    '''
    if not op.exists(filename):
        raise FileNotFoundError("No such file: '%s'" % filename)
+
+    if op.getsize(filename) <= 0:
+        raise FileEmptyError("Given file is empty: '%s'" % filename)


Perhaps we can use ImageFileError here, rather than defining a new exception? And I think the message could be shortened to "Empty file: '%s'" % filename.

raamana · 2018-03-19T20:35:13Z

I get the following, which talks about Cannot work out file type which makes me think about incorrect header, not empty file.

nibabel.filebasedimages.ImageFileError: Cannot work out file type of "/home/praamana/dev/visualqc/example_datasets/empty.nii"

codecov-io · 2018-03-19T20:38:16Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
+ Coverage   90.91%   90.91%   +<.01%     
==========================================
  Files          84       84              
  Lines       10661    10665       +4     
  Branches     1764     1764              
==========================================
+ Hits         9692     9696       +4     
  Misses        644      644              
  Partials      325      325

Impacted Files	Coverage Δ
nibabel/loadsave.py	`85.71% <100%> (+0.56%)`	⬆️

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 1140e18...a974b70. Read the comment docs.

effigies · 2018-03-20T01:19:10Z

This looks reasonable to me. I think you could quickly add a test in test_loadsave.py. The following would exercise the new code and verify that you're getting a useful error message:

def test_empty_load():
    with InTemporaryDirectory():
        open('empty.nii', 'w').close()
        with assert_raises(ImageFileError) as cm:
            load('empty.nii')
    assert_true(cm.exception.args[0].startswith('Empty file: '))

matthew-brett

Seems fine to me - one small question.

matthew-brett · 2018-03-20T08:45:53Z

nibabel/tests/test_loadsave.py

+    with InTemporaryDirectory():
+        open('empty.nii', 'w').close()
+        with assert_raises(ImageFileError) as err:
+            load('empty.nii')


I'm surprised this works on Python 3 - I thought that the err variable went out of scope.

It seems to work - didn’t dig into why

From the docs:

Signature: unittest.TestCase.assertRaises(self, expected_exception, *args, **kwargs) Docstring: Fail unless an exception of class expected_exception is raised by the callable when invoked with specified positional and keyword arguments. If a different type of exception is raised, it will not be caught, and the test case will be deemed to have suffered an error, exactly as for an unexpected exception. If called with the callable and arguments omitted, will return a context object used like this:: with self.assertRaises(SomeException): do_something() An optional keyword argument 'msg' can be provided when assertRaises is used as a context object. The context manager keeps a reference to the exception as the 'exception' attribute. This allows you to inspect the exception after the assertion:: with self.assertRaises(SomeException) as cm: do_something() the_exception = cm.exception self.assertEqual(the_exception.error_code, 3) File: /anaconda3/lib/python3.6/unittest/case.py Type: function

effigies

LGTM.

effigies · 2018-03-20T18:01:12Z

nibabel/tests/test_loadsave.py

+    with InTemporaryDirectory():
+        open('empty.nii', 'w').close()
+        with assert_raises(ImageFileError) as err:
+            load('empty.nii')


From the docs:

Signature: unittest.TestCase.assertRaises(self, expected_exception, *args, **kwargs) Docstring: Fail unless an exception of class expected_exception is raised by the callable when invoked with specified positional and keyword arguments. If a different type of exception is raised, it will not be caught, and the test case will be deemed to have suffered an error, exactly as for an unexpected exception. If called with the callable and arguments omitted, will return a context object used like this:: with self.assertRaises(SomeException): do_something() An optional keyword argument 'msg' can be provided when assertRaises is used as a context object. The context manager keeps a reference to the exception as the 'exception' attribute. This allows you to inspect the exception after the assertion:: with self.assertRaises(SomeException) as cm: do_something() the_exception = cm.exception self.assertEqual(the_exception.error_code, 3) File: /anaconda3/lib/python3.6/unittest/case.py Type: function

effigies · 2018-03-22T16:49:00Z

@matthew-brett Okay to merge?

matthew-brett · 2018-03-22T18:19:43Z

Yes, looks good - merging - thanks.

raamana added 7 commits August 2, 2017 00:52

first implementation of aseg stats reader

e06ed5e

Merge branch 'master' of github.com:nipy/nibabel

83b55a2

updating docs and improving implementation

870bb1d

first stab at a simple test

e4825f4

basic implementation of aparc.stats reader (returning everything)

af4e046

detecting if the input is empty early on! Apparently no one needed it ;)

a4f8c3c

merging with upstream master to undo changes not ready to PR/merge

a01855b

removing unnecessary space that nibabel checks dont like

3e3fe30

effigies reviewed Mar 19, 2018

View reviewed changes

Using another existing exception, and shorter message

d207360

better file checks - path.exists is try/except for os.stat anyways!

a974b70

matthew-brett reviewed Mar 20, 2018

View reviewed changes

effigies approved these changes Mar 20, 2018

View reviewed changes

matthew-brett merged commit b53a452 into nipy:master Mar 22, 2018

nibotmi mentioned this pull request Mar 22, 2018

ENH: Accept pathlib.Path objects where filenames are accepted #610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

detecting if the input is empty early on! #611

detecting if the input is empty early on! #611

Uh oh!

raamana commented Mar 19, 2018

Uh oh!

coveralls commented Mar 19, 2018 •

edited

Loading

Uh oh!

effigies left a comment

Uh oh!

effigies Mar 19, 2018

Uh oh!

effigies Mar 19, 2018

Uh oh!

raamana commented Mar 19, 2018

Uh oh!

codecov-io commented Mar 19, 2018 •

edited

Loading

Uh oh!

effigies commented Mar 20, 2018

Uh oh!

matthew-brett left a comment

Uh oh!

matthew-brett Mar 20, 2018

Uh oh!

raamana Mar 20, 2018

Uh oh!

effigies Mar 20, 2018

Uh oh!

effigies left a comment

Uh oh!

effigies Mar 20, 2018

Uh oh!

effigies commented Mar 22, 2018

Uh oh!

matthew-brett commented Mar 22, 2018

Uh oh!

Uh oh!


		if op.getsize(filename) <= 0:
		raise FileEmptyError("Given file is empty: '%s'" % filename)

detecting if the input is empty early on! #611

detecting if the input is empty early on! #611

Uh oh!

Conversation

raamana commented Mar 19, 2018

Uh oh!

coveralls commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

effigies Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

raamana commented Mar 19, 2018

Uh oh!

codecov-io commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

effigies commented Mar 20, 2018

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

matthew-brett Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

raamana Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

effigies Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

effigies commented Mar 22, 2018

Uh oh!

matthew-brett commented Mar 22, 2018

Uh oh!

Uh oh!

coveralls commented Mar 19, 2018 •

edited

Loading

codecov-io commented Mar 19, 2018 •

edited

Loading