Sage: Ticket #24884: Matrix-related fixes in differential geometry
https://trac.sagemath.org/ticket/24884
<p>
These are changes which are needed because of <a class="closed ticket" href="https://trac.sagemath.org/ticket/24742" title="enhancement: New MatrixArgs object to deal with constructing matrices (closed: fixed)">#24742</a>.
</p>
<ol><li><code>src/sage/schemes/riemann_surfaces/riemann_surface.py</code>: <a class="closed ticket" href="https://trac.sagemath.org/ticket/24742" title="enhancement: New MatrixArgs object to deal with constructing matrices (closed: fixed)">#24742</a> will disallow passing strings to the matrix constructor. It's a rarely used feature and hard to get right properly. The fix is easy: pass actual elements of the ring instead of the generator names. This is clearly the right thing to do.
</li></ol><ol start="2"><li><code>src/sage/tensor/modules/comp.py</code>: I needed to change <code>_get_list()</code> but I have to say that I don't understand what the code is supposed to do. I wrote the code the way I did mainly to pass doctests. I also added some documentation to reflect better what the <code>_get_list()</code> method does. I also got rid of the evil <code>try</code>/<code>except</code> block trying to construct a matrix but happily returning a list if that failed.
</li></ol><ol start="3"><li><code>src/sage/manifolds/differentiable/metric.py</code>: this is a consequence of the change in 2. and it looks like a sensible change.
</li></ol>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/24884
Trac 1.1.6jdemeyerFri, 02 Mar 2018 10:06:22 GMTbranch set
https://trac.sagemath.org/ticket/24884#comment:1
https://trac.sagemath.org/ticket/24884#comment:1
<ul>
<li><strong>branch</strong>
set to <em>u/jdemeyer/matrix_related_fixes_in_differential_geometry</em>
</li>
</ul>
TicketjdemeyerFri, 02 Mar 2018 10:11:19 GMTstatus, description changed; commit set
https://trac.sagemath.org/ticket/24884#comment:2
https://trac.sagemath.org/ticket/24884#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>bd532ab6734f5a87df0c237f75105c92c1158c3d</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/24884?action=diff&version=2">diff</a>)
</li>
</ul>
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=bd532ab6734f5a87df0c237f75105c92c1158c3d"><span class="icon"></span>bd532ab</a></td><td><code>Matrix-related fixes in differential geometry</code>
</td></tr></table>
TicketjdemeyerFri, 02 Mar 2018 10:13:08 GMTdescription changed
https://trac.sagemath.org/ticket/24884#comment:3
https://trac.sagemath.org/ticket/24884#comment:3
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/24884?action=diff&version=3">diff</a>)
</li>
</ul>
TicketjdemeyerFri, 02 Mar 2018 10:14:26 GMTdescription changed
https://trac.sagemath.org/ticket/24884#comment:4
https://trac.sagemath.org/ticket/24884#comment:4
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/24884?action=diff&version=4">diff</a>)
</li>
</ul>
TickettscrimFri, 02 Mar 2018 22:45:26 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/24884#comment:5
https://trac.sagemath.org/ticket/24884#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Travis Scrimshaw</em>
</li>
</ul>
<p>
Changes LGTM (Eric, if you see something you do not like, feel free to revert).
</p>
TicketegourgoulhonFri, 02 Mar 2018 22:56:44 GMTstatus changed
https://trac.sagemath.org/ticket/24884#comment:6
https://trac.sagemath.org/ticket/24884#comment:6
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_info</em>
</li>
</ul>
TicketegourgoulhonFri, 02 Mar 2018 23:12:44 GMT
https://trac.sagemath.org/ticket/24884#comment:7
https://trac.sagemath.org/ticket/24884#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:5" title="Comment 5">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Changes LGTM (Eric, if you see something you do not like, feel free to revert).
</p>
</blockquote>
<p>
Hi Travis, I was just looking to the ticket, at the same time as you apparently! The change in the doctest of <code>src/sage/manifold/differentiable/metric.py</code> (point 3 in the ticket description) is not desirable: it results from a forced conversion of SymPy to <code>SR</code> due to the change in <code>src/sage/tensor/modules/comp.py</code> (point 2). Jeroen, the line
</p>
<pre class="wiki">return matrix(SR, resu)
</pre><p>
forces the conversion of elements of the list <code>resu</code> to <code>SR</code> elements, while they can actually belong to something else, like SymPy. Is there a good reason for this? Do we have now to explicitly specify the ring as the first argument of <code>matrix</code>? Since SymPy elements do not constitute a ring in Sage, one cannot construct a matrix for them. I plan to improve this in the short future, by implementing a ring of SymPy coordinate functions. But for the time being, I would say it is preferable to return a list of SymPy elements instead of a matrix of <code>SR</code> elements.
</p>
TicketjdemeyerSat, 03 Mar 2018 08:36:10 GMT
https://trac.sagemath.org/ticket/24884#comment:8
https://trac.sagemath.org/ticket/24884#comment:8
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:7" title="Comment 7">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Jeroen, the line
</p>
<pre class="wiki">return matrix(SR, resu)
</pre><p>
forces the conversion of elements of the list <code>resu</code> to <code>SR</code> elements, while they can actually belong to something else, like SymPy. Is there a good reason for this?
</p>
</blockquote>
<p>
I just did that to be consistent with the documentation. The documentation never said "return a matrix, except when using the SymPy backend". I did make an exception for the <code>str</code> formatter, because that was documented (not in this method but elsewhere).
</p>
<blockquote class="citation">
<p>
Do we have now to explicitly specify the ring as the first argument of <code>matrix</code>?
</p>
</blockquote>
<p>
No, that won't be required. But again, it's good for consistency that you always get the same kind of object back (a symbolic matrix).
</p>
<blockquote class="citation">
<p>
I would say it is preferable to return a list of SymPy elements instead of a matrix of <code>SR</code> elements.
</p>
</blockquote>
<p>
That is fine for me, but then SymPy elements should <em>always</em> return a list. The main thing that I do not like about the current code is this <code>try</code>/<code>except</code> block:
</p>
<div class="wiki-code"><div class="code"><pre> <span class="k">try</span><span class="p">:</span>
<span class="k">for</span> i <span class="ow">in</span> <span class="nb">range</span><span class="p">(</span><span class="bp">self</span><span class="o">.</span>_dim<span class="p">):</span>
<span class="k">for</span> j <span class="ow">in</span> <span class="nb">range</span><span class="p">(</span><span class="bp">self</span><span class="o">.</span>_dim<span class="p">):</span>
a <span class="o">=</span> resu<span class="p">[</span>i<span class="p">][</span>j<span class="p">]</span>
<span class="k">if</span> <span class="nb">hasattr</span><span class="p">(</span>a<span class="p">,</span> <span class="s">'_express'</span><span class="p">):</span>
resu<span class="p">[</span>i<span class="p">][</span>j<span class="p">]</span> <span class="o">=</span> a<span class="o">.</span>expr<span class="p">()</span>
resu <span class="o">=</span> matrix<span class="p">(</span>resu<span class="p">)</span> <span class="c"># for a nicer output</span>
<span class="k">except</span> <span class="ne">TypeError</span><span class="p">:</span>
<span class="k">pass</span>
</pre></div></div><p>
With this code, you really have no idea whether a matrix or a list will be returned. And if it's a matrix, you don't know over which base ring.
</p>
<p>
Travis, what do you think?
</p>
TicketegourgoulhonSat, 03 Mar 2018 14:12:51 GMTstatus, commit, branch changed
https://trac.sagemath.org/ticket/24884#comment:9
https://trac.sagemath.org/ticket/24884#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
changed from <em>bd532ab6734f5a87df0c237f75105c92c1158c3d</em> to <em>6ae84f147a2209e0396d508c77b4c86e706f3227</em>
</li>
<li><strong>branch</strong>
changed from <em>u/jdemeyer/matrix_related_fixes_in_differential_geometry</em> to <em>public/manifolds/matrix_related_fixes_in_differential_geometry</em>
</li>
</ul>
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=6ae84f147a2209e0396d508c77b4c86e706f3227"><span class="icon"></span>6ae84f1</a></td><td><code>Improve treatment of matrix output in sage.tensor.modules.comp.Components._get_list.</code>
</td></tr></table>
TicketegourgoulhonSat, 03 Mar 2018 14:39:24 GMT
https://trac.sagemath.org/ticket/24884#comment:10
https://trac.sagemath.org/ticket/24884#comment:10
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:8" title="Comment 8">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
I just did that to be consistent with the documentation. The documentation never said "return a matrix, except when using the SymPy backend". I did make an exception for the <code>str</code> formatter, because that was documented (not in this method but elsewhere).
</p>
</blockquote>
<p>
I agree the documentation of the method <code>_get_list</code> is pretty poor (but this is a "private" (underscored) method). Sorry about this. Note that the class <code>Components</code> to which it belongs does not know anything about SR or SymPy: it manipulates only elements of a ring. The SymPy or <code>str</code> objects appear only through the output formatter.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Do we have now to explicitly specify the ring as the first argument of <code>matrix</code>?
</p>
</blockquote>
<p>
No, that won't be required. But again, it's good for consistency that you always get the same kind of object back (a symbolic matrix).
</p>
</blockquote>
<p>
OK, thanks.
</p>
<blockquote class="citation">
<p>
That is fine for me, but then SymPy elements should <em>always</em> return a list. The main thing that I do not like about the current code is this <code>try</code>/<code>except</code> block:
With this code, you really have no idea whether a matrix or a list will be returned. And if it's a matrix, you don't know over which base ring.
</p>
</blockquote>
<p>
I agree, this piece of code was not very good. Actually it pertains to the early time of the SageManifolds project, where the elements stored in <code>Components</code> where not guaranteed to belong to any ring. This is no longer the case for the <code>Components</code> used in tensor calculus: they all belong to the algebra of scalar fields and, after being processed by <code>self._output_formatter</code>, they belong to the ring of chart functions, <code>sage.manifolds.chart_func.ChartFunctionRing</code> whatever their internal representation (<code>SR</code> or Sympy). I've therefore modified the code of <code>Components._get_list</code> to
</p>
<pre class="wiki"> if self._nid == 2:
# 2-dim case: convert to matrix for a nicer output
from sage.matrix.constructor import matrix
from sage.structure.element import parent
from sage.categories.rings import Rings
if parent(resu[0][0]) in Rings():
return matrix(resu)
return resu
</pre><p>
In this way, there is no special treatment for strings or whatever else that is not a ring, since the test <code>if parent(resu[0][0]) in Rings()</code> will return false in this case. For tensor field components, the treatment is now at the level of <code>ChartFunctionRing</code> (there is no longer any reference to the internal representation via <code>_express</code> and <code>expr()</code>), so it works for both <code>SR</code> and SymPy. In particular the doctest in <code>src/sage/manifolds/differentiable/metric.py</code> now displays a matrix of SymPy objects, and no longer a list, which is an improvement! Thanks for having triggered it!
</p>
<p>
I've pushed these changes in the new branch <code>public/manifolds/matrix_related_fixes_in_differential_geometry</code>, which is based on your branch (simply one commit ahead). I am attaching it to the ticket.
</p>
TicketegourgoulhonSat, 03 Mar 2018 16:07:47 GMT
https://trac.sagemath.org/ticket/24884#comment:11
https://trac.sagemath.org/ticket/24884#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:10" title="Comment 10">egourgoulhon</a>:
</p>
<p>
An alternative to
</p>
<blockquote class="citation">
<pre class="wiki"> if parent(resu[0][0]) in Rings():
return matrix(resu)
</pre></blockquote>
<p>
is
</p>
<pre class="wiki"> R = parent(resu[0][0])
if R in Rings():
return matrix(R, resu)
</pre><p>
I don't know which form is preferable. Note that in the latter, the ring is passed as the first argument to <code>matrix</code>.
</p>
TickettscrimSat, 03 Mar 2018 22:05:15 GMT
https://trac.sagemath.org/ticket/24884#comment:12
https://trac.sagemath.org/ticket/24884#comment:12
<p>
IMO, a more ideal form is
</p>
<div class="wiki-code"><div class="code"><pre> R <span class="o">=</span> parent<span class="p">(</span>resu<span class="p">[</span><span class="mi">0</span><span class="p">][</span><span class="mi">0</span><span class="p">])</span>
<span class="k">if</span> R <span class="ow">in</span> Rings<span class="p">():</span>
<span class="k">return</span> MatrixSpace<span class="p">(</span>R<span class="p">,</span><span class="mi">2</span><span class="p">)(</span>resu<span class="p">)</span>
</pre></div></div><p>
Given <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:7" title="Comment 7">comment:7</a>
</p>
<blockquote class="citation">
<p>
I plan to improve this in the short future, by implementing a ring of SymPy coordinate functions
</p>
</blockquote>
<p>
I would also add a comment to the code saying to remove the <code>Rings</code> check once SymPy coordinate functions are a ring.
</p>
<p>
I think this also addresses Jeroen's comment about code flow clarity and the code LBTM (B for better). Any other comments Jeroen?
</p>
TicketjdemeyerSun, 04 Mar 2018 08:34:01 GMT
https://trac.sagemath.org/ticket/24884#comment:13
https://trac.sagemath.org/ticket/24884#comment:13
<p>
Instead of asking whether the parent is a ring, it seems more sensible to check for a Sage <code>Element</code>. i.e. I would suggest to replace
</p>
<pre class="wiki">parent(x) in Rings()
</pre><p>
by
</p>
<pre class="wiki">isinstance(x, Element)
</pre>
TicketjdemeyerSun, 04 Mar 2018 08:45:09 GMT
https://trac.sagemath.org/ticket/24884#comment:14
https://trac.sagemath.org/ticket/24884#comment:14
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:12" title="Comment 12">tscrim</a>:
</p>
<blockquote class="citation">
<p>
IMO, a more ideal form is
</p>
<div class="wiki-code"><div class="code"><pre> R <span class="o">=</span> parent<span class="p">(</span>resu<span class="p">[</span><span class="mi">0</span><span class="p">][</span><span class="mi">0</span><span class="p">])</span>
<span class="k">if</span> R <span class="ow">in</span> Rings<span class="p">():</span>
<span class="k">return</span> MatrixSpace<span class="p">(</span>R<span class="p">,</span><span class="mi">2</span><span class="p">)(</span>resu<span class="p">)</span>
</pre></div></div></blockquote>
<p>
What if the different elements have different parents? Is there a guarantee that the parent of <code>resu[0][0]</code> is the same as the parent of <code>resu[0][1]</code>? So <code>matrix(resu)</code> will auto-detect the correct parent (using the coercion model) and that looks better.
</p>
<p>
Of course the same argument could be made for the <code>isinstance(resu[0][0], Element)</code> check. But it seems less likely to mix Elements and non-Elements than to mix different parents.
</p>
TicketjdemeyerSun, 04 Mar 2018 09:17:01 GMTstatus changed
https://trac.sagemath.org/ticket/24884#comment:15
https://trac.sagemath.org/ticket/24884#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
TicketegourgoulhonSun, 04 Mar 2018 14:35:02 GMT
https://trac.sagemath.org/ticket/24884#comment:16
https://trac.sagemath.org/ticket/24884#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:12" title="Comment 12">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I would also add a comment to the code saying to remove the <code>Rings</code> check once SymPy coordinate functions are a ring.
</p>
</blockquote>
<p>
Actually, with the latest version, it works with SymPy, i.e. it does generate a matrix, the elements of which are chart functions, which are displayed as SymPy objects when SymPy is used as the symbolic engine (see the matrix containing <code>x**2</code> and <code>y**2</code> instead of <code>x^2</code> and <code>y^2</code> in the doctest in <code>metric.py</code>).
The <code>Rings</code> check is then for other possible cases, not involving chart functions (I don't have any precise use case at the moment, except for the <code>str</code> output formatter used as illustration in the doctests). SymPy was a problem in the previous version of the code, where the matrix construction was attempted directly at the level of symbolic representations, either <code>SR</code> or SymPy. Now the matrix construction is at the (higher) level of chart functions, and, thanks to you IIRC ;-), chart functions now forms a ring, so that the matrix construction always succeeds.
</p>
TicketegourgoulhonSun, 04 Mar 2018 14:47:16 GMT
https://trac.sagemath.org/ticket/24884#comment:17
https://trac.sagemath.org/ticket/24884#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:14" title="Comment 14">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
What if the different elements have different parents? Is there a guarantee that the parent of <code>resu[0][0]</code> is the same as the parent of <code>resu[0][1]</code>? So <code>matrix(resu)</code> will auto-detect the correct parent (using the coercion model) and that looks better.
</p>
</blockquote>
<p>
OK.
</p>
<blockquote class="citation">
<p>
Of course the same argument could be made for the <code>isinstance(resu[0][0], Element)</code> check. But it seems less likely to mix Elements and non-Elements than to mix different parents.
</p>
</blockquote>
<p>
A priori, all elements of <code>resu</code> should have the same parent: this is guaranted if no output formatter is used: the parent is nothing but the ring on which the <code>Components</code> object is constructed. If some output formatter is used, it is likely that its output is of a fixed type, hence giving rise to the same parent (in the extended sense, i.e. the parent default to the Python type if the object has no Sage Parent), like for instance the <code>str</code> output formatter. My concern with the <code>Element</code> check is that it does not guarantee that the parent is a ring. If it is not, then the matrix construction will fail.
</p>
TicketjdemeyerMon, 05 Mar 2018 11:46:06 GMTstatus, author changed
https://trac.sagemath.org/ticket/24884#comment:18
https://trac.sagemath.org/ticket/24884#comment:18
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
changed from <em>Jeroen Demeyer</em> to <em>Jeroen Demeyer, Eric Gourgoulhon</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:17" title="Comment 17">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
If it is not, then the matrix construction will fail.
</p>
</blockquote>
<p>
I didn't know that, but you are right. If you ask me, that's a silly artificial restriction.
</p>
<p>
In that case, maybe Eric's code is fine. Travis, what do you think?
</p>
TickettscrimMon, 05 Mar 2018 12:04:22 GMT
https://trac.sagemath.org/ticket/24884#comment:19
https://trac.sagemath.org/ticket/24884#comment:19
<p>
My understanding is that they should all be of the same parent. Yet, I don't hold a stake in this code, so I don't really care either way. I doubt this will ever make a difference in the wild (barring very esoteric uses).
</p>
TicketegourgoulhonMon, 05 Mar 2018 16:15:10 GMT
https://trac.sagemath.org/ticket/24884#comment:20
https://trac.sagemath.org/ticket/24884#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/24884#comment:19" title="Comment 19">tscrim</a>:
</p>
<blockquote class="citation">
<p>
My understanding is that they should all be of the same parent. Yet, I don't hold a stake in this code, so I don't really care either way. I doubt this will ever make a difference in the wild (barring very esoteric uses).
</p>
</blockquote>
<p>
Yes, I think it's pretty safe to leave the code in the current state.
</p>
TicketjdemeyerTue, 06 Mar 2018 11:17:31 GMTstatus changed
https://trac.sagemath.org/ticket/24884#comment:21
https://trac.sagemath.org/ticket/24884#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
I'm taking that as positive review :-)
</p>
TicketvbraunThu, 08 Mar 2018 00:02:39 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/24884#comment:22
https://trac.sagemath.org/ticket/24884#comment:22
<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>branch</strong>
changed from <em>public/manifolds/matrix_related_fixes_in_differential_geometry</em> to <em>6ae84f147a2209e0396d508c77b4c86e706f3227</em>
</li>
</ul>
Ticket