ENH: Minor presentation enhancement to AffineMap object

[raamana]

Description

Hello Everyone, thanks for dipy, and the registration parts of it, which I'm starting to play with it. As I tried to follow the example, I realized printing the AffineMap instance does not print the matrix, but its class/ref info:

 In[22]: affine
Out[22]: <dipy.align.imaffine.AffineMap at 0x10fd39320>

 In[23]: print(affine)
Out[23]: <dipy.align.imaffine.AffineMap object at 0x10fd39320>

which was not the intuitive, and with dir(affine) I figured I need to do affine.affine and there is no affine.get_affine() which I'd expect when a corresponding setter affine.set_affine() is defined:

 In[24]: affine.affine
Out[24]: 
array([[  9.46639056e-01,  -2.23138057e-03,  -6.39246469e-03,
          2.83507206e+01],
       [  2.22793137e-02,   1.00133362e+00,  -5.69381045e-02,
         -3.51607532e+01],
       [  1.18980288e-02,   5.37301282e-02,   9.49774845e-01,
          1.93589177e+01],
       [  0.00000000e+00,   0.00000000e+00,   0.00000000e+00,
          1.00000000e+00]])

So I suggest we implement the __format__ and __str__ for this object, and similar objects, and define AffineMap.get_affine() or a AffineMap.get_matrix() to make it more intuitive and complete.

This is not an issue or bug, and would not affect the functionality at all, just a minor suggestion. Please close it if this has been discussed already (my search didn't reveal so). Happy to send in a PR if considered helpful.

[raamana]

This also calls for implementation of other useful magic methods such as __mul__, which enables composition of multiple transformations in a more intuitive manner:

composite = rigid*affine
composite.transform(moving)

which otherwise needs to be achieved with

affine.transform(rigid.transform(moving))

Maybe, the composition of multiple transformations is implemented elsewhere?

[skoudoro]

Hi @raamana, thank you for this nice issue :-)

I think you can create a PR concerning __format__, __str__ , and get_affine.

Concerning transformation composition, it is a great idea but we need to think a little more about it. I really would like to have @omarocegueda opinion with this point. Indeed, more intuitive manner can be more confusing too. We will get back to you ASAP.

[skoudoro]

Concerning get_affine, I still wonder why we do not use @property. I prefer this way than get/set method

[matthew-brett]

I don't remember the details, but I bet it was something to do with : https://github.com/nipy/nibabel/wiki/property_manifesto

In this case, if you modify the values in place, it won't affect the return value, which breaks the 'like an attribute' contract. So this isn't too surprising:

>>> aff = obj.get_affine()
>>> aff[0, 0] = 99
>>> np.all(obj.get_affine() == aff)
False

This is more suprising:

>>> aff = obj.affine
>>> aff[0, 0] = 99
>>> np.all(obj.affine == aff)
False

[skoudoro]

Wow, thank you @matthew-brett, really nice information. Now, I will pay more intention for this feature

[raamana]

Sure @skoudoro, I will send in a PR for __format__ etc.

Yeah, implementing the composition needs to be discussed carefully..

PS: broadening the scope to even to a much higher-level, this also calls for a standalone registration library, serving all the nipy or scipy ecosystem.. I'm aware of nireg, and I know you guys have discussed this before at length.. Not trying to open it up again, just mentioning and linking the need for broad approach for registration in the python ecosystem.

[raamana]

Hi @matthew-brett , I'm not sure I am following you 100%. As we all know, the get/set behaviour of the attribute or the property would ultimately depend on implementation details.. If we choose to return "value" of the attribute in getter (instead of a reference), changes to it outside the object wouldn't be reflected in subsequent getter calls. if that use case is important, we can choose to return the reference, this needs to be discussed though.

[matthew-brett]

Yes, sure, if you return a reference, and you're happy for that to be modified outside the object, then a property is fine, because it will then behave like an attribute.

[skoudoro]

PS: broadening the scope to even more very high-level, this also calls for a standalone registration library, serving all the nipy or scipy ecosystem.. I'm aware of nireg, and I know you guys have discussed this before at length.. Not trying to open it up again, linking the need for broad approach for registraion in the python ecosystem.

We are in discussion of different strategy @raamana. Expect the unexpected :-)

For the moment, Let's focus on the PR

[arokem]

On Mon, Mar 5, 2018 at 8:44 AM, Serge Koudoro ***@***.***> wrote: PS: broadening the scope to even more very high-level, this also calls for a standalone registration library, serving all the nipy or scipy ecosystem.. I'm aware of nireg, and I know you guys have discussed this before at length.. Not trying to open it up again, linking the need for broad approach for registraion in the python ecosystem. We are in discussion of different strategy @raamana <https://github.com/raamana>. Expect the unexpected :-)

Where is that discussion taking place? I would love to be able to weigh in on this. I have been supporting several groups that are using the Dipy registration tools outside of a diffusion MRI context, and would like to be able to anticipate further developments. Telling my collaborators to "expect the unexpected" is unlikely to make them feel comfortable (I know -- you were probably only joking). For the moment, Let's focus on the PR

…

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1442 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNoKm0HpyYs4ozjlzVxjlnm1lBkyaks5tbWtrgaJpZM4SbWtc> .

[matthew-brett]

I've made an issue for the registration discussion #1444 .

[skoudoro]

I have been recently discussing with Elef about this @arokem. Nothing that you already don't know. But we should talk about this again. The viz project will be independent first.

[arokem]

OK -- great! It sounded so ominous when you said it that way...

Would we split out the viz module on the next release cycle? I guess we can pick that discussion up once we have this one out. Looks like it's pretty close!

[raamana]

So, when we define affine.get_affine() and affine.set_affine(), I don't think we should retain affine.affine, and keep it private with affine._affine

And, if we decide to keep it, we should decorate it with @property and implement a corresponding @affine.setter (decorating current set_affine), which handles assignment and validity etc.. thoughts?

[skoudoro]

fixed by #1445, closing