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: |
Description (last modified by )
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
- trac_10053_equality_testing_forms.patch
Attachments (1)
Change History (14)
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
comment:3 follow-up: ↓ 4 Changed 11 years ago by
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
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: ↓ 6 Changed 11 years ago by
- 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
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 11 years ago by
- 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: ↓ 8 Changed 11 years ago by
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
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
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
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
- 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
- Milestone set to sage-4.6
comment:13 Changed 11 years ago by
- Merged in set to sage-4.6.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
According to python documentation for eq (http://docs.python.org/reference/datamodel.html#object.__eq), you should also define ne for != comparison.