scipion-em / scipion-em-relion

Plugin to use Relion SPA programs within the Scipion framework
GNU General Public License v3.0
3 stars 0 forks source link

relion.tests.test_convert_relion.TestReconstruct fails #206

Closed azazellochg closed 4 years ago

azazellochg commented 4 years ago

Either something is wrong or just a px vs Angstroms issue in 3.1

azazellochg commented 4 years ago

wrong image size is fixed, now only TestReconstruct.test_reconstRotandShiftFlip is failing

delarosatrevin commented 4 years ago

@rmarabini Could you please help us with this issue? You wrote these convert tests and you might find more easily what is failing.

rmarabini commented 4 years ago

hi @delarosatrevin I am looking into it.

rmarabini commented 4 years ago

Hi @delarosatrevin and @azazellochg,

 I have good news and bad news The good ones is that the test fails when there is a flip in a set of angles assigned to projections. As far as I know nobody uses flips in this situation, flips are used for  2D align but not for 3D reconstruction. That is,   in practical terms this error has no consequence.

Nevertheless I would like to understand what is going on. The second matrix in mList has something wrong. I mean the determinant of a rotation matrix must be 1 (or -1 if there is a reflection) but the determinant of this matrix is 0.63. My guess is that this has not been working for a long while, in particular since we stop using xmipp for angle conversion an we started to use transformation.py. Flip may have different definition in both implementations.

OK, I will think a little more.

Roberto
delarosatrevin commented 4 years ago

@rmarabini, Thanks a lot for looking into this. It is good to know that the error is not so much relevant, still would be good to understand. Please keep us posted.

rmarabini commented 4 years ago

hi again Hi @delarosatrevin and @azazellochg,,

I have looked to the test more carefully and I think this is what is going on:

The test fails when processing the matrix

             [[-0.04341204, -0.82959837,  0.5566704,   7.42774284],  # -50, -40,-30
              [-0.90961589,  0.26325835,  0.3213938, -20.82490128],
              [0.41317591,  0.49240388,  0.76604444,  3.33947946],
              [0.,          0.,          0.,          1.]],

which has determinant -1 so there is a flip (also known as reflection). In the old xmipp we had in the function transformationMatrix2Parameters3D

flip = tmpMatrix.det3x3() < 0; if (flip) { dMij(eulerMatrix, 0, 0) = -1.; dMij(eulerMatrix, 0, 1) = -1.; dMij(eulerMatrix, 0, 2) *= -1.; } Euler_matrix2angles(eulerMatrix, rot, tilt, psi);

that is, if the determinant is negative then invert the sign of the first matrix column and then call to Euler_matrix2angles. This is not longer the case. I think we just call euler_from_matrix(matrix, axes='szyz') that is equivalent to Euler_matrix2angles(matrix, rot, tilt, psi);

Of course we need to take into account flip when going back. xmipp does:

  if (flip) // flip is here a parameter
{
    dMij(A, 0, 0) *= -1.;
    dMij(A, 0, 1) *= -1.;
    if (dim == 3)
        dMij(A, 0, 2) *= -1.;
}

but we cannot do this in scipion since flip is not stored. That, if we convert from matrix (that may store flip) to euler angles either we create an extra parameter for the flip o there is no way to save the flip information

My suggestion would be to drop flip support in 3D since nobody uses it. If we want we may modify "geometryFromMatrix" with something like if np.linalg.det(matrix) < 1: print("HORROR det of 3D transformation matrix is negative") exit(0)

Another possibility, that I do not think is the right way to go, would be to modified the function that convert matrices to angles so the flip information is saved.

that all folks

 Roberto
delarosatrevin commented 4 years ago

Thanks a lot, @rmarabini !!! I agree with you that we should not complicate things more and we should not support flip in 3D. Is the determinant already computed in the geometryFromMatrix function?

More practically, should we just remove this test from relion.convert tests?

rmarabini commented 4 years ago

Hi @delarosatrevin ,

I do not think the determinant is computed. although it would be trivial to do so

import numpy as np np.linalg.det(a)

where a is the transformation matrix.

Yes, remove (or comment out) the test

delarosatrevin commented 4 years ago

Thanks @rmarabini I was asking if it was already computed because of performance. So I think will be better to just ignore flip in 3D.

azazellochg commented 4 years ago

Hi @rmarabini thank you so much for digging into this! I removed the test 9859952530d665e63bd856e835d171176492f752