Equality testing instead of comparisons in differential forms code

jvkersch commented 14 years ago

This patch replaces the comparison member function in CoordinatePatch and DifferentialForms by equality checking (__eq__ instead __cmp__), as there is generally no canonical way of making sense of the comparison between instances of those classes.

This is related to #10041.

Apply

patch from #10041
attachment: trac_10053_equality_testing_forms.patch

CC: @jasongrout @sagetrac-mhampton @nilesjohnson @qed777 @jdemeyer

Component: symbolics

Keywords: forms, comparison, equality

Author: Joris Vankerschaver

Reviewer: Niles Johnson

Merged: sage-4.6.rc0

Issue created by migration from https://trac.sagemath.org/ticket/10053

jasongrout commented 14 years ago

comment:2

According to python documentation for __eq__ (http://docs.python.org/reference/datamodel.html#object.__eq__), you should also define __ne__ for != comparison.

jasongrout commented 14 years ago

comment:3

Try again: According to python docs for !eq! (http://docs.python.org/reference/datamodel.html#object.__eq!), you should define !ne!__ as well for != comparison.

If the objects are not hashable, you might also set !hash!=None (see the note about that function at http://docs.python.org/reference/datamodel.html#object.__hash!__)

jvkersch commented 14 years ago

comment:4

Replying to @jasongrout:

Try again: According to python docs for !eq! (http://docs.python.org/reference/datamodel.html#object.__eq!), you should define !ne!__ as well for != comparison.

Done. I didn't overload the hashing operator as this seems to work well (objects that are equal when compared have the same hash values).

9343d2e0-59ba-4406-bd4f-c78e4cf1230e commented 14 years ago

comment:5

With this patch and the one at #10041, sage passes all (-long) doctests (linux, Red Hat Enterprise).

At #10041, jason commented that comparison of tuples of symbolic variables is not well-defined, and suggested using strings instead. That has been done in coordinate_patch.py and differential_forms.py, but not in differential_form_element.py -- there, comparison uses __dict__. So is the following expected to work on all systems?

sage: F = DifferentialForms(); F
Algebra of differential forms in the variables x, y, z
sage: var('x,y,z')
(x, y, z)
sage: f = DifferentialForm(F, 2)
sage: f[1,2] = x; f
x*dy/\dz
sage: g = DifferentialForm(F, 2)
sage: g[0, 2] = y
sage: g[1, 2] = 2*x; g
2*x*dy/\dz + y*dx/\dz

sage: f == g
False
sage: g == g
True

Note:

sage: g.__dict__
{'_components': {(1, 2): 2*x, (0, 2): y}, '_degree': 2}
sage: f.__dict__
{'_components': {(1, 2): x}, '_degree': 2}
sage: type(g.__dict__['_components'][(0,2)])
<type 'sage.symbolic.expression.Expression'>

9343d2e0-59ba-4406-bd4f-c78e4cf1230e commented 14 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,8 @@
 This patch replaces the comparison member function in `CoordinatePatch` and `DifferentialForms` by equality checking (`__eq__` instead `__cmp__`), as there is generally no canonical way of making sense of the comparison between instances of those classes.

 This is related to #10041.
+
+## Apply
+
+* patch from #10041
+* [attachment: trac_10053_equality_testing_forms.patch](https://github.com/sagemath/sage-prod/files/10651074/trac_10053_equality_testing_forms.patch.gz)

9343d2e0-59ba-4406-bd4f-c78e4cf1230e commented 14 years ago

Reviewer: Niles Johnson

jvkersch commented 14 years ago

Attachment: trac_10053_equality_testing_forms.patch.gz

jvkersch commented 14 years ago

comment:6

Replying to @nilesjohnson:

Note:

sage: g.__dict__
{'_components': {(1, 2): 2*x, (0, 2): y}, '_degree': 2}
sage: f.__dict__
{'_components': {(1, 2): x}, '_degree': 2}
sage: type(g.__dict__['_components'][(0,2)])
<type 'sage.symbolic.expression.Expression'>

Thanks for noting this -- I agree that this code would give the same kind of problems as before. I've uploaded a new patch, where __eq__ essentially iterates over the underlying dictionary, converts the values to strings and does the comparison on the strings. Secondly, in the internal function _cleanup() there was some code if fun == 0, which I've replaced by if fun.is_zero(). I think this addresses all cases of symbolic expressions appearing in equalities. One downside however is that the current implementation is somewhat slower than the previous one. I have a few ideas of how to address this, but I feel this issue is better left to a different patch.

9343d2e0-59ba-4406-bd4f-c78e4cf1230e commented 14 years ago

comment:7

Replying to @jvkersch:

Thanks for noting this -- I agree that this code would give the same kind of problems as before.

Does it really!? Thinking more about this last night, I started to wonder if this is really a bug with comparison of symbolic expressions, not with differential forms. Before we move on with this, would you mind doing a little checking to see if converting to strings really is the best way to compare symbolic expressions? (If it is, then I would say '==' is broken for symbolic expressions.)

jvkersch commented 14 years ago

comment:8

Replying to @nilesjohnson:

Does it really!? Thinking more about this last night, I started to wonder if this is really a bug with comparison of symbolic expressions, not with differential forms. Before we move on with this, would you mind doing a little checking to see if converting to strings really is the best way to compare symbolic expressions? (If it is, then I would say '==' is broken for symbolic expressions.)

As far as I can tell, the comparison code in sage/symbolic/expression.pyx just calls _richcmp_ in sage/structure/element.pyx, which seems fine. After reading the bug reports here and in #10041, I thought the issue was that writing anything like x == 0 merely defined a symbolic equation (instead of evaluating directly to something boolean). I could understand this (and I think it is the intended behavior, no?), so I changed all symbolic comparisons to comparisons of string expressions, or wrote something like (lhs - rhs).is_zero(), although the latter seems to be slower.

I would love to get to the bottom of this. Specifically, I'm still confused as to why my original code (comparing tuples of symbolic variables) seemed to work fine on Mac but gave different answers on Linux. It feels like it should fail consistently across all platforms :)

jpflori commented 14 years ago

comment:9

As far as symbolic variables are concerned, pynac can compare then.

Currently it does so by comparing the underlying strings, but it could change soon because Burcin and I are working on the orders used within pynac (see http://groups.google.com/group/pynac-devel/browse_thread/thread/a36020bf9208bf08 , however you'll still get consistent equality...).

However, when using operators <, ==, etc. or _richcmp_ you get symbolic equalities and inequalities.

You can use !cmp! (symbolic/expression.pyx) or cmp to actually compare them (what is currently done by comparing strings).

If I understand correctly that calls _cmp (structure/elemet.pyx) which calls _cmp_c_impl (symbolic/expression.pyx) which calls compare of pynac (defined in libs/ginac/decl.pxi).

I don't know what is done with the tuples...

jvkersch commented 14 years ago

comment:10

Thanks Jean-Pierre, for providing that link and for your comments on this matter. With this in mind, I think that this patch and #10041 address all of the symbolic equality issues that arose in #9650 and subsequent discussions:

When dealing with symbolic expressions, we compare the corresponding string representations (this is also how pynac does it) or call (lhs - rhs).is_zero();
For tuples of symbolic string expressions, we convert everything to strings and compare the resulting tuples of strings (this is the approach taken in #10041 to establish equality of two instances of CoordinatePatch);
For dictionaries whose values are symbolic expressions, we iterate over the dictionary, convert the values to strings, and compare as we go along. Using itertools this can be done quite efficiently, so I don't see any room for improvement now. Comparing dictionaries of symbolic expressions comes up when establishing equality of differential forms, which is the subject of the current patch.

9343d2e0-59ba-4406-bd4f-c78e4cf1230e commented 14 years ago

comment:11

Ok, I'm ready to give this a positive review: the patch(es) pass all long doctests, documentation builds well. Moreover, comparison of symbolic expressions is work in progress, so comparing as strings seems to be the best current solution. jpflori suggested using cmp to compare symbolic expressions, but this is what lead to inconsistent behavior for comparison of tuples of symbolic expressions (see #10041).

Note that forms are appropriately simplified before comparison:

sage: F = DifferentialForms(); F
Algebra of differential forms in the variables x, y, z
sage: var('x,y,z')
(x, y, z)
sage: sage: g = DifferentialForm(F, 2)
sage: sage: g[0, 2] = y
sage: sage: g[1, 2] = 2*x; g
2*x*dy/\dz + y*dx/\dz
sage: g = DifferentialForm(F, 2)
sage: g[0, 2] = y
sage: g[1, 2] = 2*x; g
2*x*dy/\dz + y*dx/\dz
sage: f = DifferentialForm(F, 2)
sage: f[0, 2] = sqrt(4)*y/2
sage: f[1, 2] = -1 - x^2 + (x+1)^2; f
((x + 1)^2 - x^2 - 1)*dy/\dz + y*dx/\dz
sage: f == g
True

I will also now mark #10041 as positive review.

e14f4152-4982-4ace-8c95-73a0599b109b commented 13 years ago

Merged: sage-4.6.rc0

sagemath / sage

Equality testing instead of comparisons in differential forms code #10053

Apply