I2PC / xmipp

Xmipp is a suite of image processing programs, primarily aimed at single-particle 3D electron microscopy.
http://biocomputingunit.es/
GNU General Public License v3.0
38 stars 15 forks source link

"xmipp_volume_align / xmipp_volume_align _prog" gives wrong SHIFT values! #213

Closed MohamadHarastani closed 4 years ago

MohamadHarastani commented 5 years ago

In the volume_align_prog, the following change was introduced by someone and should be reverted immediately: lines (438-442): A = A + Aaux; A(0,0) -= 1; A(1,1) -= 1; A(2,2) -= 1; A(3,3) -= 1; These lines should replaced with: A = A * Aaux;

In the devel branch, the volume_align_prog and its corresponding volume_align_prog_main are in separate folders (/xmipp/src/xmipp/libraries/reconstruction/) and (/xmipp/applications/programs/volume_align_prog/) respectively. However, in the remaining branches, they are combined and only volume_align_prog_main exists.

This separation causes the following warning which is disturbing: Segmentation fault (core dumped) or: /bin/xmipp_volume_align: line 3: 6331 Segmentation fault (core dumped) $XMIPP_HOME/bin/xmipp_volume_align_prog "$@" This warning is not effecting the functionality of this program (tested with --frm option).

DStrelak commented 5 years ago

Hello @MohamadHarastani,

Thanks for reporting this issue and sorry for the troubles it caused. I have already addressed the second part (segmentation fault, see https://github.com/I2PC/xmipp/pull/214).

As for the first part, the change happened two years ago by @jvargas (see https://github.com/I2PC/scipion/commit/c14452d5184be8bac4873c47c12cc71caffdeb30) Can you elaborate more on why is the current state wrong?

Thanks.

DStrelak commented 4 years ago

ping

MohamadHarastani commented 4 years ago

Hello @DStrelak, I will check this during this week and let you know. Thanks for your patience.

DStrelak commented 4 years ago

ping

MohamadHarastani commented 4 years ago

I will sort this out this week. Thanks again for your patience.

MohamadHarastani commented 4 years ago

Hi, I have checked this issue with a test on 200 synthetic volumes (randomly rotating and shifting, then retrieving the values and comparing with the ground-truth). Unfortunately, this program is still giving wrong shift values. The program (here in lines 447 - 452) displays the alignment parameters that another program "Xmipp_transform_geometry" requires to do the alignment. Displayed parameters are XYZ shifts and three angles. Where the displayed angles are correct, the displayed shifts are not. Just reverting this change (as I indicated in the original issue report) solves this problem. Thanks for waiting.

DStrelak commented 4 years ago

Hello @MohamadHarastani , we had a look at it and we concluded that you are probably right. If nothing else, the current way of adding the matrices and then deducting the identity is not very elegant.

We have a test for Scipion (./scipion test xmipp3.tests.test_protocol_structure_mapping.TestStructureMapping). It is sort of expected to test this program, but currently it is not very well designed.

I wonder if you could come up with two structures, that are related by shift (not too large) and rotation (not too large). The structures should be similar, but not identical, so that the normal mode makes them similar. We could use them to refactor the test, and then we can change the matrix multiplication accordingly.

If you need anything, feel free to contact me here or via email.

Thanks

MohamadHarastani commented 4 years ago

Hello @DStrelak, I believe that when one uses this program (xmipp_volume_align) with the option (--apply), the program is finding and applying the correct rigid body alignment parameters (i.e. whatever is in the matrix best_align is correct). The problem might be on what is stored using the options "--copyGeo" and "--verbose". So I think that the tests with structure mapping will be okay (because it only uses the option "--apply"). Regarding the two structures that are slightly different using some nma deformations: I have already done something similar, I can forward these to you whenever needed. Regards,

DStrelak commented 4 years ago

Please do forward me those. If you can alter the test yourself though, it would be much appreciated.

DStrelak commented 4 years ago

Hi @MohamadHarastani,

can we close this ticket as solved?

MohamadHarastani commented 4 years ago

Hi @DStrelak Sure. I forwarded the tests a while ago to you to verify. However, it is solved. Regards,