Sage: Ticket #10053: Equality testing instead of comparisons in differential forms code
https://trac.sagemath.org/ticket/10053
<p>
This patch replaces the comparison member function in <code>CoordinatePatch</code> and <code>DifferentialForms</code> by equality checking (<code>__eq__</code> instead <code>__cmp__</code>), as there is generally no canonical way of making sense of the comparison between instances of those classes.
</p>
<p>
This is related to <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a>.
</p>
<h2 id="Apply">Apply</h2>
<ul><li>patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10053/trac_10053_equality_testing_forms.patch" title="Attachment 'trac_10053_equality_testing_forms.patch' in Ticket #10053">trac_10053_equality_testing_forms.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10053/trac_10053_equality_testing_forms.patch" title="Download"></a>
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10053
Trac 1.1.6jvkerschSat, 02 Oct 2010 00:16:16 GMTstatus changed
https://trac.sagemath.org/ticket/10053#comment:1
https://trac.sagemath.org/ticket/10053#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketjasonSat, 02 Oct 2010 01:28:18 GMT
https://trac.sagemath.org/ticket/10053#comment:2
https://trac.sagemath.org/ticket/10053#comment:2
<p>
According to python documentation for <span class="underline">eq</span> (<a class="ext-link" href="http://docs.python.org/reference/datamodel.html#object.__eq"><span class="icon"></span>http://docs.python.org/reference/datamodel.html#object.__eq</a><span class="underline">), you should also define </span>ne<span class="underline"> for != comparison.
</span></p>
TicketjasonSat, 02 Oct 2010 01:32:06 GMT
https://trac.sagemath.org/ticket/10053#comment:3
https://trac.sagemath.org/ticket/10053#comment:3
<p>
Try again: According to python docs for __eq__ (<a class="ext-link" href="http://docs.python.org/reference/datamodel.html#object.__eq__"><span class="icon"></span>http://docs.python.org/reference/datamodel.html#object.__eq</a>__), you should define __ne__ as well for != comparison.
</p>
<p>
If the objects are not hashable, you might also set __hash__=None (see the note about that function at <a class="ext-link" href="http://docs.python.org/reference/datamodel.html#object.__hash__"><span class="icon"></span>http://docs.python.org/reference/datamodel.html#object.__hash</a>__)
</p>
TicketjvkerschSun, 03 Oct 2010 21:08:05 GMT
https://trac.sagemath.org/ticket/10053#comment:4
https://trac.sagemath.org/ticket/10053#comment:4
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10053#comment:3" title="Comment 3">jason</a>:
</p>
<blockquote class="citation">
<p>
Try again: According to python docs for __eq__ (<a class="ext-link" href="http://docs.python.org/reference/datamodel.html#object.__eq__"><span class="icon"></span>http://docs.python.org/reference/datamodel.html#object.__eq</a>__), you should define __ne__ as well for != comparison.
</p>
</blockquote>
<p>
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).
</p>
TicketnilesTue, 05 Oct 2010 15:24:19 GMTstatus, description changed; reviewer set
https://trac.sagemath.org/ticket/10053#comment:5
https://trac.sagemath.org/ticket/10053#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
<li><strong>reviewer</strong>
set to <em>Niles Johnson</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10053?action=diff&version=5">diff</a>)
</li>
</ul>
<p>
With this patch and the one at <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a>, sage passes all (<code>-long</code>) doctests (linux, Red Hat Enterprise).
</p>
<p>
At <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a>, jason <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/10041#comment:3"><span class="icon"></span>commented</a> that comparison of tuples of symbolic variables is not well-defined, and suggested using strings instead. That has been done in <code>coordinate_patch.py</code> and <code>differential_forms.py</code>, but not in <code>differential_form_element.py</code> -- there, comparison uses <code>__dict__</code>. So is the following expected to work on all systems?
</p>
<pre class="wiki">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
</pre><p>
Note:
</p>
<pre class="wiki">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'>
</pre>
TicketjvkerschWed, 06 Oct 2010 04:33:20 GMTattachment set
https://trac.sagemath.org/ticket/10053
https://trac.sagemath.org/ticket/10053
<ul>
<li><strong>attachment</strong>
set to <em>trac_10053_equality_testing_forms.patch</em>
</li>
</ul>
TicketjvkerschWed, 06 Oct 2010 04:38:57 GMTstatus changed
https://trac.sagemath.org/ticket/10053#comment:6
https://trac.sagemath.org/ticket/10053#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10053#comment:5" title="Comment 5">niles</a>:
</p>
<blockquote class="citation">
<p>
Note:
</p>
<pre class="wiki">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'>
</pre></blockquote>
<p>
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 <code>__eq__</code> essentially iterates over the underlying dictionary, converts the values to strings and does the comparison on the strings. Secondly, in the internal function <code>_cleanup()</code> there was some code <code>if fun == 0</code>, which I've replaced by <code>if fun.is_zero()</code>. 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.
</p>
TicketnilesWed, 06 Oct 2010 11:41:44 GMT
https://trac.sagemath.org/ticket/10053#comment:7
https://trac.sagemath.org/ticket/10053#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10053#comment:6" title="Comment 6">jvkersch</a>:
</p>
<blockquote class="citation">
<p>
Thanks for noting this -- I agree that this code would give the same kind of problems as before.
</p>
</blockquote>
<p>
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 '<code>==</code>' is broken for symbolic expressions.)
</p>
TicketjvkerschThu, 07 Oct 2010 05:15:28 GMT
https://trac.sagemath.org/ticket/10053#comment:8
https://trac.sagemath.org/ticket/10053#comment:8
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10053#comment:7" title="Comment 7">niles</a>:
</p>
<blockquote class="citation">
<p>
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 '<code>==</code>' is broken for symbolic expressions.)
</p>
</blockquote>
<p>
As far as I can tell, the comparison code in <code>sage/symbolic/expression.pyx</code> just calls <code>_richcmp_</code> in <code>sage/structure/element.pyx</code>, which seems fine. After reading the bug reports here and in <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a>, I thought the issue was that writing anything like <code>x == 0</code> 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 <code>(lhs - rhs).is_zero()</code>, although the latter seems to be slower.
</p>
<p>
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 :)
</p>
TicketjpfloriFri, 08 Oct 2010 09:05:21 GMT
https://trac.sagemath.org/ticket/10053#comment:9
https://trac.sagemath.org/ticket/10053#comment:9
<p>
As far as symbolic variables are concerned, pynac can compare then.
</p>
<p>
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 <a class="ext-link" href="http://groups.google.com/group/pynac-devel/browse_thread/thread/a36020bf9208bf08"><span class="icon"></span>http://groups.google.com/group/pynac-devel/browse_thread/thread/a36020bf9208bf08</a> , however you'll still get consistent equality...).
</p>
<p>
However, when using operators <, ==, etc. or _richcmp_ you get symbolic equalities and inequalities.
</p>
<p>
You can use __cmp__ (symbolic/expression.pyx) or cmp to actually compare them (what is currently done by comparing strings).
</p>
<p>
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).
</p>
<p>
I don't know what is done with the tuples...
</p>
TicketjvkerschSun, 10 Oct 2010 05:40:46 GMT
https://trac.sagemath.org/ticket/10053#comment:10
https://trac.sagemath.org/ticket/10053#comment:10
<p>
Thanks Jean-Pierre, for providing that link and for your comments on this matter. With this in mind, I think that this patch and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a> address all of the symbolic equality issues that arose in <a class="closed ticket" href="https://trac.sagemath.org/ticket/9650" title="enhancement: Adding support for differential forms (closed: fixed)">#9650</a> and subsequent discussions:
</p>
<ul><li>When dealing with symbolic expressions, we compare the corresponding string representations (this is also how pynac does it) or call (lhs - rhs).is_zero();
</li><li>For tuples of symbolic string expressions, we convert everything to strings and compare the resulting tuples of strings (this is the approach taken in <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a> to establish equality of two instances of <code>CoordinatePatch</code>);
</li><li>For dictionaries whose values are symbolic expressions, we iterate over the dictionary, convert the values to strings, and compare as we go along. Using <code>itertools</code> 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.
</li></ul>
TicketnilesMon, 11 Oct 2010 21:32:46 GMTstatus changed
https://trac.sagemath.org/ticket/10053#comment:11
https://trac.sagemath.org/ticket/10053#comment:11
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
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 <code>cmp</code> to compare symbolic expressions, but this is what lead to inconsistent behavior for comparison of tuples of symbolic expressions (see <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a>).
</p>
<p>
Note that forms are appropriately simplified before comparison:
</p>
<pre class="wiki">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
</pre><p>
I will also now mark <a class="closed ticket" href="https://trac.sagemath.org/ticket/10041" title="defect: Doctest failure in sage/tensor/differential_forms.py (closed: fixed)">#10041</a> as positive review.
</p>
TicketmpatelThu, 21 Oct 2010 07:40:32 GMTmilestone set
https://trac.sagemath.org/ticket/10053#comment:12
https://trac.sagemath.org/ticket/10053#comment:12
<ul>
<li><strong>milestone</strong>
set to <em>sage-4.6</em>
</li>
</ul>
TicketmpatelThu, 21 Oct 2010 08:42:39 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10053#comment:13
https://trac.sagemath.org/ticket/10053#comment:13
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.6.rc0</em>
</li>
</ul>
Ticket