rt_mx inconsistencies
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
The math is lost on me, but is it possible that revision 15658 solves the
problem (in part or in whole)?
http://cctbx.svn.sourceforge.net/viewvc/cctbx/trunk/cctbx/sgtbx/space_group.cpp?r1=15082&r2=15658&sortby=rev
-Nat
On Wed, Jul 18, 2012 at 9:31 AM, Luc Bourhis
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 [email protected] http://phenix-online.org/mailman/listinfo/cctbxbb
Hi Nat,
The math is lost on me, but is it possible that revision 15658 solves the problem (in part or in whole)?
It should indeed but it is an ugly hack. I mean, using exceptions to implement the logic normally expressed with "if ... then" ... So I stand by the opinion expressed above. Luc
On Wed, Jul 18, 2012 at 9:58 AM, Luc Bourhis
The math is lost on me, but is it possible that revision 15658 solves the problem (in part or in whole)?
It should indeed but it is an ugly hack. I mean, using exceptions to implement the logic normally expressed with "if ... then" ... So I stand by the opinion expressed above.
I personally have no objections, as long as it doesn't break Phenix, but could anyone else (especially Phil Evans) who actually uses these features comment? -Nat
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 [email protected] http://phenix-online.org/mailman/listinfo/cctbxbb
Is there a reason why "==" test of rt_mx or rot_mx would do anything other than test them element by element? Or is this a side-effect of providing a rank order? (at least for the "rotation" part of rt_mx: I suppose that for symmetry operators a translation of +0.5 == -0.5) My regression test was a distillation of a real life crash reported by Graeme Winter 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 [email protected] http://phenix-online.org/mailman/listinfo/cctbxbb
Hi Phil,
Is there a reason why "==" test of rt_mx or rot_mx would do anything other than test them element by element? Or is this a side-effect of providing a rank order?
I hinted at the answer in my original email. Let me elaborate. The new comparisons < and > rely on the geometrical elements of the compared rt_mx instances. That is sensible since, in any application I can think of, one will want e.g. higher orders to come first, then for the same orders, rotations about cell axis a to come before rotation about cell axis b, etc. I don't remember in details the implementation of those inequality operators, so I am making it up for the sake of illustration! Using element-wise comparison for those > and < operators is pretty pointless on the contrary. But then, for any objects for which x > y and x < y are defined, we must have the following equivalences: x <= y equivalent to not(x > y); x >=y equivalent to not(x < y); x == y equivalent to x >= y and x <= y. Those are general mathematical rules that have nothing to do with crystallography. Thus it is highly sensible, having defined operators > and < to implement operator== using those rules. This is precisely what has been done in the revision that broke the code in your regression test. Now, the key question we must asked ourselves is: because one particular algorithm needs an ordering on the set of rt_mx, is it a good idea to implement that ordering in the rt_mx class? Related question: is it likely that another algorithm may need another ordering? or that one may want to tweak the ordering for one algorithm? If the answer to any of the last two question is a lukewarm "yes", then the answer to the first question is a resounding "no". I had faced the very same issue when I worked on reconstructing the space-group from FFT map and my take at it was to wrap rt_mx instances in a proxy and to define the order on that proxy. That way, I can use different orderings by using different proxy classes and the essential brick rt_mx can keep its straightforward definition of comparison. I haven't studied in enough depth the symmetry-equivalent pair interaction table generation algorithm to be sure that this approach can work but I am reasonably optimistic that this pattern is applicable. Sorry for the exaggeratedly long email(s), best wishes, Luc
Thank you for that clear explanation Phil On 19 Jul 2012, at 10:34, Luc Bourhis wrote:
Hi Phil,
Is there a reason why "==" test of rt_mx or rot_mx would do anything other than test them element by element? Or is this a side-effect of providing a rank order?
I hinted at the answer in my original email. Let me elaborate.
The new comparisons < and > rely on the geometrical elements of the compared rt_mx instances. That is sensible since, in any application I can think of, one will want e.g. higher orders to come first, then for the same orders, rotations about cell axis a to come before rotation about cell axis b, etc. I don't remember in details the implementation of those inequality operators, so I am making it up for the sake of illustration! Using element-wise comparison for those > and < operators is pretty pointless on the contrary.
But then, for any objects for which x > y and x < y are defined, we must have the following equivalences: x <= y equivalent to not(x > y); x >=y equivalent to not(x < y); x == y equivalent to x >= y and x <= y. Those are general mathematical rules that have nothing to do with crystallography.
Thus it is highly sensible, having defined operators > and < to implement operator== using those rules. This is precisely what has been done in the revision that broke the code in your regression test.
Now, the key question we must asked ourselves is: because one particular algorithm needs an ordering on the set of rt_mx, is it a good idea to implement that ordering in the rt_mx class? Related question: is it likely that another algorithm may need another ordering? or that one may want to tweak the ordering for one algorithm? If the answer to any of the last two question is a lukewarm "yes", then the answer to the first question is a resounding "no". I had faced the very same issue when I worked on reconstructing the space-group from FFT map and my take at it was to wrap rt_mx instances in a proxy and to define the order on that proxy. That way, I can use different orderings by using different proxy classes and the essential brick rt_mx can keep its straightforward definition of comparison. I haven't studied in enough depth the symmetry-equivalent pair interaction table generation algorithm to be sure that this approach can work but I am reasonably optimistic that this pattern is applicable.
Sorry for the exaggeratedly long email(s), best wishes,
Luc
_______________________________________________ cctbxbb mailing list [email protected] http://phenix-online.org/mailman/listinfo/cctbxbb
participants (3)
-
Luc Bourhis -
Nathaniel Echols -
Phil Evans