BF - only update header if affine not close

[matthew-brett]

Discussion at #55. Because affine
in nifti1 stored as float32, using == to check if affine has changed
would give false positives, comparing to float64 input. Brendan Moloney
kindly suggested allclose fix.

[matthew-brett]

@moloney @MrBago - any comments here?

[MrBago]

Hi Matthew,
Sorry for not responding sooner. I think we should also add a convenience function to help make this more intuitive. I've written something here if you want to take a look, https://github.com/MrBago/nibabel/tree/update-header-allclose.

This function is meant to handle two use cases:

1. It seems useful to know that the affine that I set in the image is the one that will be stored to disk if I save.
2. after calling set_affine, I know that the sform/qform codes will not be changed unexpectedly when I save.

I considered some other variants on this function, for example one that raises an error if the affine cannot be set. Let me know what you think.

On a tangent, should we let people create an image with an integer array as the affine?

Bago

[moloney]

@MrBago - I see a couple of issues with the set_affine method:

1. If the private attribute '_affine' is None, then it will raise an exception on the last line.
2. It seems like this method is intended to be general, but it is implemented only in the Nifti1Pair class. If a method like this is included it should probably be available in all image classes.
3. I think your use cases are already handled without it. At least in the case of Nifti format, whatever is set in the image affine will now end up stored on disk. If the user wants to do something format specific like store two different affines in the qform and sform, then they just need to call the format specific functions 'set_sform' and 'set_qform'.

[MrBago]

I'll fix to code to handle _affine is None better, thanks for catching that. I threw this together really quickly so I'm totally open to changing it (and writing tests) before doing a pull request, but I think something like this would be useful.

The nifti sform and qform fields cannot store any 4x4 matrix so there is no way to guarantee that what's in the affine will end up on disk. You must do img.get_affine()[:] = hdr.get_best_affine() to ensure that the image is self-consistent, ie that the affine in self.get_affine() will make it to disk, especially if you're trying to create an image with sform=0. Because the current implementation requires self-consistency in order to preserve the sform_code and qform_code on save it seems especially useful to have a function that can update the affine, ensure self-consistency and potentially inform the user if their affine was changed in the process.

I'm not opposed to creating similar methods for other image classes if people think they would be useful, but one thing at a time please.

[moloney]

@matthew-brett - I have played around with this a bit. Seems good to me.

[matthew-brett]

Here's a compare view, just because I spent a few minutes finding how to do it:

MrBago/nibabel@matthew-brett:update-header-allclose...MrBago:update-header-allclose

I would like to avoid a set_affine method especially just for the Nifti images. Specifically, I want to simplify the interface to use an .affine property.

I'm also a bit uncomfortable about having a function change signature between image types, as I think this one must.

To be clear about your usecase - you want to be able to set the sform and the affine in one call? And you want to be warned if the affine changed as it was being set?

In practice I think the affine can only (allclose) change if code for sform is 0, and for qform is non-zero.

What do you think should happen for sform code = 0, qform code = 0?

How about the following counterproposal? Two methods unique to nifti:

set_sform(aff, code='aligned', update_affine=True)
set_qform(aff, code='aligned', update_affine=True)

with a return value as for your function?

[moloney]

@MrBago - Isn't the 3x4 sform matrix sufficient for all practical uses? Maybe we could have 'set_sform' raise an exception if the affine uses the last row. Similarly 'set_qform' could raise an exception/warning if the affine transform in non-rigid and the code is not "Unknown".

[MrBago]

We don't need to call it set_affine, I can see why that might be an issue. I want a method such shat img.SomeMethod(affine) returns an image so that img.affine == img.header.get_best_affine(). I think this is useful because when we save to disk img.affine get's lost and it might be useful to know that your image has the same affine now as it will have later when someone reads it from disk. Only the first three rows of the img.affine matrix get saved in the sform fileds so a nifti image can can "self inconsistent" even if sform_code=1. For example:

>>> new_affine = np.arange(16.).reshape(4,4)
>>> img.get_affine()[:] = new_affine
>>> img.update_header()
>>> img.get_affine()
array([[  0.,   1.,   2.,   3.],
       [  4.,   5.,   6.,   7.],
       [  8.,   9.,  10.,  11.],
       [ 12.,  13.,  14.,  15.]])
>>> img.get_header().get_best_affine()
array([[  0.,   1.,   2.,   3.],
       [  4.,   5.,   6.,   7.],
       [  8.,   9.,  10.,  11.],
       [  0.,   0.,   0.,   1.]])

I'm not sure why someone would do this, maybe by mistake. I think adding something like set_sform(aff, code='aligned', update_affine=True) to the image class would work, but I believe it would have to be a method of the image because the header knows nothing about the image it's attached to.

I believe that this type of method can happly co-exist with a img.affine property.

[MrBago]

@matthew-brett, I misunderstood your counter-proposal.
Now that I looked at it again +1.

[moloney]

@matthew-brett - One issue I see with your counter proposal:

>>> hdr = img.get_header()
>>> hdr.set_sform(my_sform, my_sform_code)
>>> hdr.set_qform(my_qform, my_qform_code)
>>> img.update_header()
>>> np.allclose(hdr.get_sform(), my_sform)
False
>>> hdr['qform_code'] == my_qform_code #Provdied my_qform_code was anything but 'unknown'
False
>>> hdr['sform_code'] == my_sform_code #Provided my_sform_code was anything but 'aligned'
False

Edit -> got that backwards, you would lose the sform transform (and potentially both codes) in this case.

[matthew-brett]

On Mon, Mar 12, 2012 at 4:05 PM, moloney
[email protected]
wrote:

@matthew-brett - One issue I see with your counter proposal:

>>> img.set_sform(my_sform, my_sform_code)
>>> img.set_qform(my_qform, my_qform_code)
>>> img.update_header()
>>> np.allclose(img.get_qform(), my_qform)
False
>>> img['qform_code'] == my_qform_code
False

Well - my plan was for 'update_affine' == True (the default) to update
the affine if and only if the modification would change the affine
when the image was loaded.

So, if the sform is defined, then

img.set_qform(my_qform, my_qform_code)

will result in no change to the affine. Have I thought that through correctly?

[MrBago]

Is this kind of what you had in mind Matthew?
MrBago/nibabel@matthew-brett:update-header-allclose...MrBago:update-header-allclose

[moloney]

Ah, I see now. Yes I think this should work.

Would the boolean return value signify whether the qform/sform was correctly set in the header, or does it signify whether the affine was set in the object? If the sform is defined then setting the qform will never set the object's affine, but it could still potentially be set correctly (without any information lost) in the header.

[matthew-brett]

On Mon, Mar 12, 2012 at 5:34 PM, moloney
[email protected]
wrote:

Ah, I see now.  Yes I think this should work.

Would the boolean return value signify whether the qform/sform was correctly set in the header, or does it signify whether the affine was set in the object?  If the sform is defined then setting the qform will never set the object's affine, but it could still potentially be set correctly (without any information lost) in the header.

Good point - I guess it would be 'correctly set in header' because
this is the only thing that can fail.

[matthew-brett]

On Mon, Mar 12, 2012 at 5:32 PM, MrBago
[email protected]
wrote:

Is this kind of what you had in mind Matthew?
MrBago/nibabel@matthew-brett:update-header-allclose...MrBago:update-header-allclose

Yes - although Brendan is right about the allclose comparing the input
affine to that in the header - don't you think?

Also, do you think it would be reasonable to allow None for the
affine, if the code is 0, and error otherwise?

Would you consider crafting some tests and submitting a pull request?

[matthew-brett]

On Mon, Mar 12, 2012 at 9:03 PM, Matthew Brett [email protected] wrote:

On Mon, Mar 12, 2012 at 5:32 PM, MrBago
[email protected]
wrote:

Is this kind of what you had in mind Matthew?
MrBago/nibabel@matthew-brett:update-header-allclose...MrBago:update-header-allclose

Yes - although Brendan is right about the allclose comparing the input
affine to that in the header - don't you think?

Also, do you think it would be reasonable to allow None for the
affine, if the code is 0, and error otherwise?

It also occurs to me that this might be confusing:

img = Nifti1Image(np.zeros((2,3,4)), np.diag([2,3,4,1]))
img.set_sform(np.eye(4), 0, update_affine=True)

What would be the affine after that? Whatever was in the qform? What
if there's nothing (code = 0) in the qform?
What results

[MrBago]

I don't see a problem with this this specific case.

>>> img = nib.Nifti1Image(np.zeros((2,3,4)), np.diag([2,3,4,1]))
>>> img.set_sform(np.eye(4), 0, update_affine=True)
>>> True
>>> img.get_affine()
array([[-2.,  0.,  0.,  1.],
       [ 0.,  3.,  0., -3.],
       [ 0.,  0.,  4., -6.],
       [ 0.,  0.,  0.,  1.]])

In general if the user sets the sform_code to 0, I think the onus on the user to either set a qform or the pixdim. Otherwise the existing qfrom/pixdim in the header should be used I think.

One question I have is, should the pixdim get updated by set_sform if qform is 0?

I didn't completely follow the discussion about comparing the input to the the header. In the example the input is compared to header.get_sform()/get_qform(), it's not clear to me what the alternative would be.

[moloney]

@MrBago - There are two questions the user may have when using the proposed set_qform / set_sform:

1. Did the sform/qform get set correctly in the header without information loss (will the affine correctly make it to disk)
2. Did the objects affine get successfully updated to the provided affine (will a subsequent call to img.get_affine() return the same thing)

In you example implementation, the return value answers question 1. If we are to go this route, I would vote for answering question 2 with the return value and answering question 1 with an exception or warning.

[moloney]

A possible alternative to the 'update_affine' argument to set_qform / set_sform would be to allow the user to explicitly choose the source for the image affine (either the sform or the qform) using a method like 'choose_affine_source'. If this is never called then the behavior would be the same as it currently is (prefer the sform, fall back to the qform). If it is called then the img.get_affine() and img.update_header() would always get or set the choosen header field. Then the set_sform / set_qform methods can just return True or False as they currently do in MrBago's example implementation.

I think this would also make it easier to work with images that have two different affines.

[matthew-brett]

I would rather avoid the chose_affine_source implementation because it's specific to nifti and adds to the image state, which is currently just (data, affine, header).

I'll draft up some tests to see if they cover the use-cases OK, and push them to this pull request. Maybe that will make the use-cases clearer.

[MrBago]

Ya I think you're right Matthew. I believe we talked about something like chose_affine_source a few months back, but it becomes very complicated very quickly. If you want help with this I can contribute but it won't be for a few weeks. We can merge this pull request in the mean time if you'd like.

[matthew-brett]

OK - I will push this one, and start another branch for the sform / qform draft