Skielex / structure-tensor

Structure tensor 2D and 3D implementation for Python.
MIT License
47 stars 14 forks source link

Multiprocessing eigenvalue order #12

Closed PaPieta closed 7 months ago

PaPieta commented 1 year ago

In the base version, the eigenvalues are returned in the ascending order (eig0<eig1<eig2). In the multiprocessing version, this order is reversed (eig0>eig1>eig2).

Attached a small example stMultiprocBug.txt

Skielex commented 1 year ago

Yeah. So, I this is actually done on purpose. I decided that it would make more sense in general to order the eigenvalues in descending order. However, I didn't want to introduce breaking changes in a minor update, so I didn't change the existing eig_special_3d function.

Unless something convinces me otherwise all functions will be changed to returning the values in descending order when I do an update with breaking changes (likely v. 1.0).

What do you think makes more sense? Ascending or descending?

PaPieta commented 1 year ago

Yes, descending order is more intuitive in general - that is what most other methods that use eigenvalue decomposition do. The only argument for the ascending order that I have is that some papers you mention and Vedrana's note on structure tensor use it.

In the end it doesn't matter too much, as long as it is clearly stated. My main issue with the current setup is that this ordering change isn't mentioned anywhere. I've spent a bit too much time trying to figure out why all of a sudden my results (e.g. linearity measure) are useless.

Skielex commented 1 year ago

Thanks a lot for the input. I'm sorry I wasted your time - it should definitely be stated in the function docstring. I'll try to get the fixed soon! As mentioned I'll probably change all functions to use descending order in version 1.0, but for now I'll probably just add the docs. I might also add the order as a function argument in the next update, but without changing the default behavior.

R-icntay commented 1 year ago

Does returning the eigen values in descending order change the following statement in one of the tutorials:

Note that the first axis of the vector correspond to (z, y, x), meaning that vec[0] is the length of the vector along the Z-axis, not the X-axis as you might expect.

Thank you..

Skielex commented 1 year ago

First of all, sorry that I haven't gotten around to fix this issue.

@R-icntay, the order of the axes (x, y, z) or (z, y, x) is not related to issue discussed here and does not depend on whether eigen values are sorted ascending or descending. Per default only the vector corresponding to the smallest eigenvalue is returned, while all three eigen values are returned. You have the option to get all three vectors by using the full argument. In that case, another axis is added to the returned vector array. The order of that axis should match that of the eigenvalues. However, this is a different axis than the one representing (z, y, x).

Skielex commented 7 months ago

Fixed in #15