Opened 11 years ago

Closed 11 years ago

#10053 closed defect (fixed)

Equality testing instead of comparisons in differential forms code

Reported by: jvkersch Owned by: burcin
Priority: major Milestone: sage-4.6
Component: symbolics Keywords: forms, comparison, equality
Cc: jason, mhampton, niles, mpatel, jdemeyer Merged in: sage-4.6.rc0
Authors: Joris Vankerschaver Reviewers: Niles Johnson
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by niles)

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

Attachments (1)

trac_10053_equality_testing_forms.patch (6.6 KB) - added by jvkersch 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by jvkersch

  • Status changed from new to needs_review

comment:2 Changed 11 years ago by jason

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

comment:3 follow-up: Changed 11 years ago by jason

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__)

comment:4 in reply to: ↑ 3 Changed 11 years ago by jvkersch

Replying to jason:

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).

comment:5 follow-up: Changed 11 years ago by niles

  • Description modified (diff)
  • Reviewers set to Niles Johnson
  • Status changed from needs_review to needs_info

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'>

Changed 11 years ago by jvkersch

comment:6 in reply to: ↑ 5 ; follow-up: Changed 11 years ago by jvkersch

  • Status changed from needs_info to needs_review

Replying to niles:

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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 11 years ago by niles

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.)

comment:8 in reply to: ↑ 7 Changed 11 years ago by jvkersch

Replying to niles:

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 :)

comment:9 Changed 11 years ago by jpflori

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...

comment:10 Changed 11 years ago by jvkersch

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.

comment:11 Changed 11 years ago by niles

  • Status changed from needs_review to positive_review

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.

comment:12 Changed 11 years ago by mpatel

  • Milestone set to sage-4.6

comment:13 Changed 11 years ago by mpatel

  • Merged in set to sage-4.6.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.