[cctbxbb] rt_mx inconsistencies

[Phil Evans]

I'm happy with this proposal
I do want r1 == m1 or m1 == m2 not to throw an exception

What I am doing is comparing a change_of_basis_op with a symmetry operator, and I want to know if they are the same: neither of these are necessarily rotations. The problem I saw was comparing these objects as rt_mx objects. Actually I am only interested in the rotation part, so I probably should have been comparing rt_mx.r objects (.r() in C++): this is what I am doing now and this works with the current version (I admit to not understanding why this should behave differently). Thus I do want rot_mx to work in the same way

For my purposes I'm not interested in ordering operators (at least I don't think so)

I would think that the equality test could be done by element, one of the advantages of storing the matrix as fixed-point

Phil

On 18 Jul 2012, at 14:31, Luc Bourhis wrote:

> Hi fellow cctbx developers,
> 
> class rt_mx, contrarily to the documentation, does not only hold rotation + translation but any transformation y = M x + t where M is an invertible matrix. As a result, rt_mx can be used to store both space group elements and change-of-basis operators (the latter are in general not rotations). Not the purest design but apart from the recurrent need of improving on the documentation, it has been all right.
> 
> (1) However, revision 15082 has introduced an inconsistency in the comparison of rt_mx instances related to that divide between rotations and non-rotations.
> Let's r1 and r2 be rotations and m1 and m2 be non-rotations. After that revision,
> 	r1 == r2 works all right
> 	r1 == m1 throws an exception
> 	m1 == m2 throws an exception
> As far as I know the problem has been first reported by Phil Evans who provided a very clear regression test.
> The reason is simple: that revision changed operator== to compare the geometric elements, assuming the compared operators are rotations (and therefore that they have an invariant axis or plane).
> 
> (2) Another side effect of revision 15082 seems to result in r1 == r2 being true even if r1.t() and r2.t() do not have the same denominator. Before that revision, this comparison used to return false. As part of that revision, one test has been changed from != to == for that very reason.
> 
> (3) revision 15082 follows revision 14990 that introduces an order on rt_mx intances. That order is used to implement a new symmetry-equivalent pair interaction table and rightly so op1 == op2 is made consistent with op1 < op2 and op1 <= op2 by revision 15082.
> 
> So I am coming to you to discuss what to do.
> 
> I am of the opinion that (1) needs fixing. I am also of the opinion that (2) is a good move. Here is what I propose to do:
> (i) Remove operator <=, >=, <, > from class rt_mx and revert operator== to what it used to be.
> (ii) Change rot_mx and tr_vec operator== to disregard the value of den()
> (iii) Introduce a proxy class that wraps around rt_mx and that obeys the ordering introduced by revision 14990
> (iv) Rewrite the implementation of  the symmetry-equivalent pair interaction table generation to use that proxy instead of directly comparing space group symmetry elements.
> 
> Basically I propose a very orthogonal design that keeps the core sgtbx pristine except for the fixing of a small but long-standing glitch while encapsulating the feature needed by one single algorithm along with that algorithm.
> 
> What do you think?
> 
> Best wishes,
> 
> Luc Bourhis
> 
> _______________________________________________
> cctbxbb mailing list
> cctbxbb at phenix-online.org
> http://phenix-online.org/mailman/listinfo/cctbxbb