ENH: Add a random affine transformation matrix generation util.

[jhlegarreta]

Add a random affine transformation matrix generation util.

Resolves #1687.

[pep8speaks]

Hello @jhlegarreta, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2019-03-16 21:25:21 UTC

[jhlegarreta]

So here it goes. Still WIP.

A few comments:

• Not sure the chosen one is the best location for the script. Reviews and comments are welcome.
• Not sure how to do the testing, since the transformation is random. An alternative would be to make the methods accept parameters, and generate random values if not provided. But this may be related to the physical meaning of the parameters, and the comment below.
• The testing does not fit into DIPY's testing framework yet.
• Not sure the unit determinant works as expected/is implemented correctly: the determinant is not 1. In any case, do we need that?
• Zooming/scaling not incorporated: do we need to add it to the affine matrix generation.
• I found a few interesting links with potentially useful tools (may be too much for the purpose of a single affine matrix transform but may be worthwhile to consider):
  • A related Python package: https://pypi.org/project/transformations/
  • A repository from @matthew-brett inspired from the above: https://github.com/matthew-brett/transforms3d
    • The way the matrices are generated in the packages above is specified in terms of math/physical parameters (e.g. angles, points, directions, etc.), while in the proposed patch, values are completely random.
  • Does not look like the easiest way to generate the rotation matrix, but I ignore whether we want to build it "this way": http://blog.lostinmyterminal.com/python/2015/05/12/random-rotation-matrix.html

Got some inspiration from here and here.

Thanks.

[codecov-io]

Codecov Report

Merging #1709 into master will increase coverage by 0.00%.
The diff coverage is 88.09%.

@@           Coverage Diff           @@
##           master    #1709   +/-   ##
=======================================
  Coverage   91.21%   91.21%           
=======================================
  Files         250      250           
  Lines       31803    31845   +42     
  Branches     3340     3342    +2     
=======================================
+ Hits        29008    29048   +40     
- Misses       2074     2077    +3     
+ Partials      721      720    -1     

Impacted Files	Coverage Δ
dipy/core/geometry.py	90.62% <86.11%> (-0.58%)	⬇️
dipy/core/tests/test_geometry.py	99.00% <100.00%> (+0.03%)	⬆️
dipy/viz/app.py	77.25% <0.00%> (+0.50%)	⬆️
dipy/segment/tests/test_bundles.py	90.47% <0.00%> (+0.95%)	⬆️

[arokem]

Hi @jhlegarreta - thanks for writing this. I think that this belongs in dipy.core.geometry.

Could you please move it there? Otherwise, this looks good to me, but it would be good to add some more testing.

[jhlegarreta]

@arokem Moved the methods to dipy.core.geometry. The unit determinant matrix generator is working. As for the transformation methods, I'm:

• still wondering whether @matthew-brett 's implementation is preferred over the current one.
• if not, I'd like to hear how do you propose to test the transformation methods.

A big thanks.

[arokem]

Sorry: would you mind pointing out where in @matthew-brett's code there is code that generates random affines? Do you mean this? Would that always create an affine?

[jhlegarreta]

@arokem Yes, that's the test. I guess it would: https://github.com/matthew-brett/transforms3d/blob/master/transforms3d/affines.py

[arokem]

I am not sure that I understand the question: "if not, I'd like to hear how do you propose to test the transformation methods." Are you asking how to test that it's really an affine?

[jhlegarreta]

@arokem Yes.

[sidkapoor97]

@arokem should the transformation matrix (rotation, shear,etc) be 3x3 or 4x4? because the 3x3 implementation can be extended for 4x4 matrices, I read the source codes for geometry.py, and most of the transformations are implemeted for 3x3

[sidkapoor97]

@arokem is this issue still useful?

[jhlegarreta]

I had this on my ToDo list since a few months. Sorry for the delay @arokem.

@sidkapoor97 Having this in DIPY may turn out to be useful at some point I guess.

[skoudoro]

What is the status of this PR @jhlegarreta? Should I tag it for this release? Next one?

[jhlegarreta]

What is the status of this PR @jhlegarreta? Should I tag it for this release? Next one?

Still on my ToDo list. Not likely to be ready for the next release.

[skoudoro]

I see here that we have a function to setup a random transformation matrix.

it will be good if you could move this function. Also, Can you provide us an update concerning this PR.

It would be really useful, I got many times this issue and recently with #2407

[skoudoro]

Hi @jhlegarreta,

We plan to talk about your PR during the DIPY online meeting tomorrow (October 7th, 2021 | 1pm EST / 7pm CET / 10am PT).

It would be great if you could attend this meeting to provide us a short update.

More information on #2398

Thank you!

[jhlegarreta]

Hi @jhlegarreta,
We plan to talk about your PR during the DIPY online meeting tomorrow (October 7th, 2021 | 1pm EST / 7pm CET / 10am PT).
It would be great if you could attend this meeting to provide us a short update.
More information on #2398
Thank you!

@skoudoro Thanks for the heads up. I have not been able to attend to the meetings lately because I have another overlapping recurrent meeting at the DIPY meetings. Unfortunately, tomorrow I still have the same issue. Happy to discuss at another time.

I have not forgotten about the issue. Still on my ToDo list.