Byteskip minus1 fixing

[tashrifbillah]

1. Fixed byteskip -1 reading issue #70

2. Added test for both nrrd and nhdr files with byte skip: -1

3. Added test data with nrrd gzip and nhdr raw files with byte skip: -1

[codecov-io]

Codecov Report

Merging #74 into master will increase coverage by 0.72%. The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   87.39%   88.12%   +0.72%     
==========================================
  Files           6        6              
  Lines         357      362       +5     
  Branches      114      117       +3     
==========================================
+ Hits          312      319       +7     
+ Misses         22       21       -1     
+ Partials       23       22       -1

Impacted Files	Coverage Δ
nrrd/reader.py	83.73% <62.5%> (+1.74%)	:arrow_up:

---

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 5310894...f82bfcb. Read the comment docs.

[tashrifbillah]

cc: @ihnorton

[tashrifbillah]

Let me know what do you think @addisonElliott

[tashrifbillah]

How does it look now? I understand for all compressed data, data.flags['WRITEABLE'] is False for some reason, but don't know why.

[tashrifbillah]

I have taken care of the previous failures. Sorry, it took me some time to realize that the version installed in my computer was different from the cloned one. This mismatch attributed to all the mysterious missing lines so far.

We should be good now.

[addisonElliott]

How does it look now? I understand for all compressed data, data.flags['WRITEABLE'] is False for some reason, but don't know why.

Take a look there: https://github.com/mhe/pynrrd/pull/68

Here is the relevant information I discovered:

Use fromstring function when dealing with compressed data because it will copy the data from the string into a Numpy array and allow the data to be editable. If you use frombuffer, then it will simply reference the memory. For a binary string, this ends up with the numpy array not being editable.

[tashrifbillah]

Done, sorry about that.

[addisonElliott]

Code looks good, just add a few quick tests to test it and we should be good to go! 👍

[tashrifbillah]

Test that it throws an exception when byte_skip is negative (but not -1) Change the byte_skip dataset to add fake data to the front of it so that you have to do a byte_skip=-1 to get it to work. Then add a test with/without byte_skip being -1 to ensure it works. Repeat the above test for raw, ASCII & GZIP

Your test data addition suggestion should be straightforward. I'll try to complete them soon.

[tashrifbillah]

Test that it throws an exception when byte_skip is negative (but not -1)

This is done using numpy.testing.assert_raises.

I am working on the rest.

[addisonElliott]

Wasn't aware of numpy.testing.assert_raises so that's great, but the NRRD test util has it's own function, see here: https://github.com/mhe/pynrrd/blob/master/nrrd/tests/test_reading.py#L122

I would use that just to be consistent.

You edited your comment but you make a good point about adding random data. Not sure...but it would be nice to verify that byte_skip=-1 is working correctly in a sense that it won't work without byte_skip = -1.

If it can't be done then that's okay, just stick with the normal tests.

[tashrifbillah]

Yeah, after putting my comment, I realized there might be a way of doing the way you suggested. So, let's see if I can come up with that design. Otherwise, you have my all ears.

[tashrifbillah]

I have the following questions:

1. Is this reassigning the file pointer to the beginning of the file after reading header?

2. If answer to 1 is yes, is this reading only numpy data from the entire file content?

[addisonElliott]

Not entirely sure but based on the comment, I think it is seeking to right where the header stops. It seems here so the next parts are where the data is located

Sounds like it was a special case added to fix something

[tashrifbillah]

Hey @addisonElliott , I have been able to form a test case that demonstrates the use of byte_skip: -1. I am at the final stage of my editing. However, can you help a little but about https://github.com/mhe/pynrrd/blob/master/nrrd/tests/test_reading.py#L122 ?

I mean how do I apply it instead of

    np.testing.assert_raises(nrrd.errors.NRRDError, nrrd.read, GZ_NIFTI_NHDR_FILE_PATH)

that I have used?

[tashrifbillah]

In my opinion, since you have used np.testing all through the test_reading.py, I think you are better off using np.testing.assert_raises.

[addisonElliott]

Yeah, I understand. The benefit of the utility assertRaises function is that you can ensure that the error message is exactly what you expect. Not sure if np.testing has that capability.

Either way works, but for consistency, just stick with the utility function.

It works like this: take that line of code, change the message to what it should be, and then inside the context of the with statement, place your core that should cause an exception

[addisonElliott]

Should be something like:

with self.assertRaises(nrrd.NRRDError, 'Message here of exception'):
    nrrd.read(GZ_NIFTI_NHDR_FILE_PATH)

[tashrifbillah]

I hope we have done what we wanted. Now Pynrrd supports byte skip: -1 even for compressed data.

[addisonElliott]

Great job @tashrifbillah!

The test cases are great, and I'm especially pleased by the byte_skip=-1 with NIFTI TAR file as the data. Great practical test.

I'm going to merge this into master and I'll wait a few weeks before publishing a new version in case any other PRs are submitted. If I forget in a few weeks, bug me somehow and I'll publish a new version.

👍

[tashrifbillah]

No problem, happy to work on it. @ihnorton may also put his two cents in when he gets back.