Add an 'axis' parameter to concat_images, plus two tests.

[bcipolli]

Trying to solve issue #207. There are now three cases:

axis is not None: concat on the specified dimension
axis is None and all images have a final dimension of 1: concat on the final dimension (don't add a new one)
axis is None and any image has a final dimension != 1: concat on a new dimension.

Existing tests covered the 3rd case. New tests were added for the first two cases.

[coveralls]

Coverage increased (+0.01%) to 94.33% when pulling c6c6a1d on bcipolli:issue-207 into 96d474c on nipy:master.

[matthew-brett]

I would prefer not to have a special case for ones on the last axis - the user can always do (when fixed) axis=-1 for that case.

[bcipolli]

Fair enough. :)

[bcipolli]

@matthew-brett Please take a look. I know np.asarray copies from lists; I believe np.concatenate must copy as well. I beefed up the test coverage as well.

[coveralls]

Coverage increased (+0.01%) to 94.33% when pulling c39caac on bcipolli:issue-207 into 96d474c on nipy:master.

[matthew-brett]

This loses the memory efficiency of the original. How about:

if axis is None:  # collect images in output array for efficiency
    out_shape = (n_imgs, ) + i0shape
    out_data = np.empty(out_shape)
else:  # collect images in list for use with np.concatenate
    out_data = [None] * n_imgs

Then you can use out_data[i] = img.get_data() in the loop.

[bcipolli]

Why is this less memory efficient? It doesn't copy anywhere. I assume that, under the covers, numpy preallocates a large array and does a memcopy. So I believe this should be very efficient.

[matthew-brett]

Sorry - I should have explained - it's memory efficient because Python can delete the memory for the individual images as it goes through the loop. Having said that, in order for that to happen, we need more logic:

if is_filename:
    del img

[bcipolli]

Got it. I have an idea how to do all the cases efficiently; I'll try it out sometime today. I'll also look into improving the test coverage and checks.

[matthew-brett]

Anything I can do to help?

[bcipolli]

Sorry, had a conference this weekend and have been doing some research. I can get get back to this on Monday and try to wrap things up!

[matthew-brett]

No problem - and thanks for your work on this.

[bcipolli]

Hope to get to this today or tomorrow.

[matthew-brett]

Great - thanks.​

[bcipolli]

@matthew-brett I've got the basic code working, but had a question while testing.

How flexible do you want concat_images to be? Specifically, if a mix of 3D and 4D images (with a unary 4th dimension, for example) are passed, it is possible to concat them--but it'd take an extra check to make it work.

Would you prefer simply to let that error, or to do the check to make sure those work together? From the testing standpoint, it's no problem to test that case as I've written things now.

[bcipolli]

Also, mixed image/filename is not allowed. Easy to extend for that as well.
Finally, I think del img is fine without the if check; it just removes the reference (doesn't force garbage collection); see https://docs.python.org/2/reference/simple_stmts.html#the-del-statement

[matthew-brett]

My instinct is to go for simpler code at the expense of not allowing obscurities like mixing 3D and 4D, or image objects and filenames. We can always add those later if it looks like a significant use-case.

[bcipolli]

@matthew-brett It made the code simpler to allow mixed than to disallow it. However, the 3D/4D possibilities did make the code more complex.

The code could lose about 5-6 lines of complexity if all images must be 3D or 4D. The testing code is going to be complex no matter what, due to the testing of all axes, None, and no argument specified.

Let me know if you'd like to trim some of the cases, but things are working as-is.

[coveralls]

Coverage increased (+0.02%) to 94.34% when pulling 47fc8f0 on bcipolli:issue-207 into 96d474c on nipy:master.

[matthew-brett]

Drop this check? I guess if they pass in an empty list they can expect an empty list back?

[bcipolli]

In the past, and currently, this throws an error. I added the check because the error did not indicate the issue clearly.

[matthew-brett]

Fair enough.

[coveralls]

Coverage increased (+0.01%) to 94.34% when pulling 0567ac3 on bcipolli:issue-207 into 96d474c on nipy:master.

[coveralls]

Coverage increased (+0.01%) to 94.34% when pulling 0567ac3 on bcipolli:issue-207 into 96d474c on nipy:master.

[bcipolli]

@matthew-brett I cut out the 3D/4D special cases and added testing for 2D and 5D.

I did have to add some complexity. I found a lot of edge cases where numpy's broadcasting, or details of np.concatenate, led to unexpected results. These are unlikely edge cases, but to do things correctly (and test generically), I have to explicitly test the shape.

The upside is that the error message will be extremely clear. The downside is that the code to check the shape properly in both axis cases was ~7 lines.

Let me know what you think! I think we're pretty close now.

[coveralls]

Coverage increased (+0.01%) to 94.34% when pulling e9298cf on bcipolli:issue-207 into 96d474c on nipy:master.

[matthew-brett]

typo rrequires

[coveralls]

Coverage increased (+0.01%) to 94.34% when pulling 84c0fd2 on bcipolli:issue-207 into cf4f946 on nipy:master.

[bcipolli]

@matthew-brett made the requested changes, sans the question about the error check.

[matthew-brett]

OK - thanks very much for all your work on this.

The last remaining thing is that we are deleting the image even if it is passed in as an image object, in the list, and that could be bad. We either want to delete the image only if we created it by loading the filename, or (probably better) we could use img.get_data(caching='unchanged') so we don't fill up the cache in the image if it is already empty, and the image objects will stay really small.

Also you kindly removed the expect_error from the try branch, but it is undefined in some tests now.

I put in a pull request to your branch just now with fixes for these, and some optional refactoring. If you prefer my version, please go ahead and merge, otherwise - fix these last things and we'll merge asap.

Thanks again...

[bcipolli]

@matthew-brett I pushed a change for expect_error issue.

As for the del, I believe it is proper--we're not telling Python to delete the image; we're simply deleting the (local) reference to the image. If the image was passed in as an object, the calling code will still have a reference and the image data will remain in memory; if it was loaded locally, then that reference is the only one, and the object will be marked for garbage collection. If that's right, then there's no need for any special logic.

Thanks for the pull request / refactor. I prefer my version with the initialization within the loop, as it potentially avoids loading the first image's data twice. Since we worked hard to improve efficiency in this version, I think that small optimization is worthwhile. But as with you, my preference isn't strong; happy to defer if you feel any obstacle.

Thanks also for your work, sorry I didn't get this done more smoothly! Let me know if, after reading these comments, you think there are still some changes to make, and I'll be glad to take care of them ASAP.

[matthew-brett]

For the del - yes, thinking about it, the loop iteration will make new reference to the object that may have its only other reference in a passed list. On the other hand, this function will fill the image array cache of the passed image objects, which may be desirable or not desirable. I think probably not.

For the load data twice if doing initialization outside the loop - could you explain? I think there's only one 'get_data' call in the code.

I prefer my code because - well who doesn't prefer their own code? - but also I have an allergy to doing i == 1 initialization in the loop that is instinctive, but this is your work, so this is your call.

[bcipolli]

On the other hand, this function will fill the image array cache of the passed image objects, which may be desirable or not desirable. I think probably not.

I believe I understand. I also believe this issue existed in the old version of the code. If so, can we open a new issue to address it?

For the load data twice if doing initialization outside the loop - could you explain? I think there's only one 'get_data' call in the code.

I probably simply don't know the nibabel code well enough. I assumed the data was loaded when load(img) was called; sounds like it's always deferred to get_data. In that case, the in-loop initialization is totally unnecessary and just more confusing.

[bcipolli]

Just merged your code; given what was discussed above, I think it solves the remaining issues. Let's be done with this! :)

[matthew-brett]

Thank you for being so patient with my nit-picks and slowness, and thanks very much for taking the time to do this. I think the test fails are spurious and it's time to merge...