Sage: Ticket #18529: Topological manifolds: basics
https://trac.sagemath.org/ticket/18529
<p>
This is the implementation of topological manifolds over a topological field <em>K</em> resulting from the <a class="extlink" href="http://sagemanifolds.obspm.fr/"><span class="icon"></span>SageManifolds project</a>. See the metaticket <a class="new ticket" href="https://trac.sagemath.org/ticket/18528" title="task: SageManifolds metaticket (new)">#18528</a> for an overview.
By <em>topological manifold over a topological field K</em> it is meant a second countable Hausdorff space <em>M</em> such that every point in <em>M</em> has a neighborhood homeomorphic to <em>K<sup>n</sup></em>, with the same nonnegative integer <em>n</em> for all points.
</p>
<p>
This tickets implements the following Python classes:
</p>
<ul><li><code>ManifoldSubset</code>: generic subset of a topological manifold (the open subsets being implemented by the subsclass <code>TopologicalManifold</code>)
<ul><li><code>TopologicalManifold</code>: topological manifold over a topological field <em>K</em>
</li></ul></li><li><code>ManifoldPoint</code>: point in a topological manifold
</li><li><code>Chart</code>: chart of a topological manifold
<ul><li><code>RealChart</code>: chart of a topological manifold over the real field
</li></ul></li><li><code>CoordChange</code>: transition map between two charts of a topological manifold
</li></ul><p>
as well as the singleton classes<code>TopologicalStructure</code> and <code>RealTopologicalStructure</code>.
</p>
<p>
<code>TopologicalManifold</code> is intended to serve as a base class for specific manifolds, like smooth manifolds (<em>K</em>=<strong>R</strong>) and complex manifolds (<em>K</em>=<strong>C</strong>). The followup ticket, implementing continuous functions to the base field, is <a class="closed ticket" href="https://trac.sagemath.org/ticket/18640" title="enhancement: Topological manifolds: scalar fields (closed: fixed)">#18640</a>.
</p>
<p>
<strong>Documentation</strong>:
The reference manual is produced by
<code>sage docbuild reference/manifolds html</code>
It can also be accessed online at <a class="extlink" href="http://sagemanifolds.obspm.fr/doc/18529/reference/manifolds/"><span class="icon"></span>http://sagemanifolds.obspm.fr/doc/18529/reference/manifolds/</a>
More documentation (e.g. example worksheets) can be found <a class="extlink" href="http://sagemanifolds.obspm.fr/documentation.html"><span class="icon"></span>here</a>.
</p>
enusSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/18529
Trac 1.1.6egourgoulhonWed, 27 May 2015 16:23:24 GMTdescription changed
https://trac.sagemath.org/ticket/18529#comment:1
https://trac.sagemath.org/ticket/18529#comment:1
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=1">diff</a>)
</li>
</ul>
TicketjhpalmieriWed, 27 May 2015 21:55:09 GMT
https://trac.sagemath.org/ticket/18529#comment:2
https://trac.sagemath.org/ticket/18529#comment:2
<p>
The phrase "manifold over a field K" sounds odd to me. Is it used in the literature? What if K is a finite field? It seems that if X is a finite discrete space, for every finite field F and for every nonnegative integer n, then X is a manifold over F of dimension n: F and n play no role. (I'm assuming that finite fields have been given the discrete topology.)
</p>
<p>
I think you might say "topological manifold over a topological field K", since obviously the topology on K is critical. Or you could omit "over a field K", and mention in the documentation that users can specify a topological field (like \CC, rather than the default \RR) if they want.
</p>
TicketegourgoulhonWed, 27 May 2015 22:27:50 GMTdescription changed
https://trac.sagemath.org/ticket/18529#comment:3
https://trac.sagemath.org/ticket/18529#comment:3
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=3">diff</a>)
</li>
</ul>
TicketegourgoulhonWed, 27 May 2015 22:32:35 GMT
https://trac.sagemath.org/ticket/18529#comment:4
https://trac.sagemath.org/ticket/18529#comment:4
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:2" title="Comment 2">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
The phrase "manifold over a field K" sounds odd to me. Is it used in the literature?
</p>
</blockquote>
<p>
Thanks for your comment. You are right: this is an abusive generalization of "manifold over R" and "manifold over C", which are used in the literature.
</p>
<blockquote class="citation">
<p>
What if K is a finite field? It seems that if X is a finite discrete space, for every finite field F and for every nonnegative integer n, then X is a manifold over F of dimension n: F and n play no role. (I'm assuming that finite fields have been given the discrete topology.)
</p>
</blockquote>
<blockquote class="citation">
<p>
I think you might say "topological manifold over a topological field K", since obviously the topology on K is critical. Or you could omit "over a field K", and mention in the documentation that users can specify a topological field (like \CC, rather than the default \RR) if they want.
</p>
</blockquote>
<p>
Thanks for your suggestion; I've modified the ticket description accordingly. I've also added what is meant by "topological manifold over a topological field K".
</p>
<p>
PS: note that the code in the associated branch is still in a very crude draft state, but should be ready for review within a few days.
</p>
TicketgitWed, 27 May 2015 22:41:10 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:5
https://trac.sagemath.org/ticket/18529#comment:5
<ul>
<li><strong>commit</strong>
changed from <em>89c063c7119f19497e3d21b2a5a9dcb0752122b0</em> to <em>5a5722b4a0ef33d8624fdd127bbb1964232ced96</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=5a5722b4a0ef33d8624fdd127bbb1964232ced96"><span class="icon"></span>5a5722b</a></td><td><code>Add doctests in classes Chart and RealChart</code>
</td></tr></table>
TicketgitThu, 28 May 2015 14:21:28 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:6
https://trac.sagemath.org/ticket/18529#comment:6
<ul>
<li><strong>commit</strong>
changed from <em>5a5722b4a0ef33d8624fdd127bbb1964232ced96</em> to <em>4f490af5fedeb0a28dd8ddab70efdab1cc64bf93</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=4f490af5fedeb0a28dd8ddab70efdab1cc64bf93"><span class="icon"></span>4f490af</a></td><td><code>Improve the documentation of coordinate charts</code>
</td></tr></table>
TicketgitThu, 28 May 2015 22:00:13 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:7
https://trac.sagemath.org/ticket/18529#comment:7
<ul>
<li><strong>commit</strong>
changed from <em>4f490af5fedeb0a28dd8ddab70efdab1cc64bf93</em> to <em>d8df59f286da79e1e103b56064fdffb702e034ce</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=d8df59f286da79e1e103b56064fdffb702e034ce"><span class="icon"></span>d8df59f</a></td><td><code>Improve the documentation of TopManifold</code>
</td></tr></table>
TicketgitFri, 29 May 2015 16:07:21 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:8
https://trac.sagemath.org/ticket/18529#comment:8
<ul>
<li><strong>commit</strong>
changed from <em>d8df59f286da79e1e103b56064fdffb702e034ce</em> to <em>fb96562c1e7d4ffc76ca87efcc15d133c6c15190</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=fb96562c1e7d4ffc76ca87efcc15d133c6c15190"><span class="icon"></span>fb96562</a></td><td><code>Open subsets of topological manifolds are now fully considered as topological manifolds.</code>
</td></tr></table>
TicketegourgoulhonFri, 29 May 2015 16:12:23 GMTdescription changed
https://trac.sagemath.org/ticket/18529#comment:9
https://trac.sagemath.org/ticket/18529#comment:9
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=9">diff</a>)
</li>
</ul>
TicketgitSat, 30 May 2015 14:10:13 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:10
https://trac.sagemath.org/ticket/18529#comment:10
<ul>
<li><strong>commit</strong>
changed from <em>fb96562c1e7d4ffc76ca87efcc15d133c6c15190</em> to <em>38c3c12cb1d9b4d52a35ad6f7a1a8d0ee666d532</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=38c3c12cb1d9b4d52a35ad6f7a1a8d0ee666d532"><span class="icon"></span>38c3c12</a></td><td><code>Improve documentation of classes TopManifold, TopManifoldSubset and TopManifoldPoint</code>
</td></tr></table>
TicketgitThu, 04 Jun 2015 13:48:12 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:11
https://trac.sagemath.org/ticket/18529#comment:11
<ul>
<li><strong>commit</strong>
changed from <em>38c3c12cb1d9b4d52a35ad6f7a1a8d0ee666d532</em> to <em>7809ebf468f80143e33830e0df75046bf191ce24</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=7809ebf468f80143e33830e0df75046bf191ce24"><span class="icon"></span>7809ebf</a></td><td><code>Minor modifications in classes TopManifold and Chart.</code>
</td></tr></table>
TicketgitThu, 18 Jun 2015 16:05:37 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:12
https://trac.sagemath.org/ticket/18529#comment:12
<ul>
<li><strong>commit</strong>
changed from <em>7809ebf468f80143e33830e0df75046bf191ce24</em> to <em>99cc8c1c94197b1648966eaa7b2a6d04ddab1efc</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=99cc8c1c94197b1648966eaa7b2a6d04ddab1efc"><span class="icon"></span>99cc8c1</a></td><td><code>Introduce Chart._init_coordinates() to simplify constructors of Chart and RealChart</code>
</td></tr></table>
TicketgitTue, 23 Jun 2015 15:39:07 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:13
https://trac.sagemath.org/ticket/18529#comment:13
<ul>
<li><strong>commit</strong>
changed from <em>99cc8c1c94197b1648966eaa7b2a6d04ddab1efc</em> to <em>26fc318bbacfc9a27049a9397d43402975525820</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=26fc318bbacfc9a27049a9397d43402975525820"><span class="icon"></span>26fc318</a></td><td><code>Modifications in CoordChange to allow for subclasses.</code>
</td></tr></table>
TicketgitFri, 26 Jun 2015 16:11:56 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:14
https://trac.sagemath.org/ticket/18529#comment:14
<ul>
<li><strong>commit</strong>
changed from <em>26fc318bbacfc9a27049a9397d43402975525820</em> to <em>a74c2e0519c166657b858be5b4b3dc4fd0145d09</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=a74c2e0519c166657b858be5b4b3dc4fd0145d09"><span class="icon"></span>a74c2e0</a></td><td><code>Add example of padic manifold</code>
</td></tr></table>
TicketgitMon, 24 Aug 2015 14:40:46 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:15
https://trac.sagemath.org/ticket/18529#comment:15
<ul>
<li><strong>commit</strong>
changed from <em>a74c2e0519c166657b858be5b4b3dc4fd0145d09</em> to <em>542b82a9ed134759fe498cd34b682b2d565061c1</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=542b82a9ed134759fe498cd34b682b2d565061c1"><span class="icon"></span>542b82a</a></td><td><code>Suppress verbose in TestSuite().run; minor improvements in documentation</code>
</td></tr></table>
TicketgitThu, 01 Oct 2015 15:33:11 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:16
https://trac.sagemath.org/ticket/18529#comment:16
<ul>
<li><strong>commit</strong>
changed from <em>542b82a9ed134759fe498cd34b682b2d565061c1</em> to <em>26c489001c4ee64a95ee70da72210e361180eeb0</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=26c489001c4ee64a95ee70da72210e361180eeb0"><span class="icon"></span>26c4890</a></td><td><code>Minor improvements in the doc of topological manifolds (basics part)</code>
</td></tr></table>
TicketgitTue, 13 Oct 2015 21:32:30 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:17
https://trac.sagemath.org/ticket/18529#comment:17
<ul>
<li><strong>commit</strong>
changed from <em>26c489001c4ee64a95ee70da72210e361180eeb0</em> to <em>be3ff7424963450c1c2dfa7ca9fbe85eaeab1162</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=be3ff7424963450c1c2dfa7ca9fbe85eaeab1162"><span class="icon"></span>be3ff74</a></td><td><code>Introduce open covers on top manifolds</code>
</td></tr></table>
TicketgitThu, 15 Oct 2015 12:37:20 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:18
https://trac.sagemath.org/ticket/18529#comment:18
<ul>
<li><strong>commit</strong>
changed from <em>be3ff7424963450c1c2dfa7ca9fbe85eaeab1162</em> to <em>4de19a74c83ac6d4d0c4da74e1d1f2afce5c3045</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=4de19a74c83ac6d4d0c4da74e1d1f2afce5c3045"><span class="icon"></span>4de19a7</a></td><td><code>Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into sage 6.9</code>
</td></tr></table>
TicketegourgoulhonThu, 15 Oct 2015 20:40:46 GMTstatus, description, milestone changed
https://trac.sagemath.org/ticket/18529#comment:19
https://trac.sagemath.org/ticket/18529#comment:19
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=19">diff</a>)
</li>
<li><strong>milestone</strong>
changed from <em>sage6.8</em> to <em>sage6.10</em>
</li>
</ul>
TicketvdelecroixSat, 17 Oct 2015 13:44:08 GMT
https://trac.sagemath.org/ticket/18529#comment:20
https://trac.sagemath.org/ticket/18529#comment:20
<p>
Why did you abreviate <code>TopologicalManifold</code>? Everywhere in Sage classes have plain names like <code>PolynomialRing</code> and not <code>PolRing</code>.
</p>
TicketegourgoulhonSat, 17 Oct 2015 14:26:37 GMT
https://trac.sagemath.org/ticket/18529#comment:21
https://trac.sagemath.org/ticket/18529#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:20" title="Comment 20">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
Why did you abreviate <code>TopologicalManifold</code>?
</p>
</blockquote>
<p>
No strong argument, except that this is shorter to write (well, thanks to the tab key this is a pretty weak argument) and it is a reminder of Top, which is the standard name for the category of topological manifolds. Similarly, we use<code>DiffManifold</code> for differentiable manifold, Diff being the standard name of the category of differentiable manifolds.
</p>
<blockquote class="citation">
<p>
Everywhere in Sage classes have plain names like <code>PolynomialRing</code> and not <code>PolRing</code>.
</p>
</blockquote>
<p>
This is indeed a strong point: naming conventions should be homogeneous all across Sage. So I am considering to change everywhere <code>TopManifold</code> to <code>TopologicalManifold</code> and <code>DiffManifold</code> to <code>DifferentiableManifold</code>.
</p>
TicketgitMon, 19 Oct 2015 09:17:06 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:22
https://trac.sagemath.org/ticket/18529#comment:22
<ul>
<li><strong>commit</strong>
changed from <em>4de19a74c83ac6d4d0c4da74e1d1f2afce5c3045</em> to <em>6dec6d592a09e56921b9a761827309dd31ae2533</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=36473057a7c5250f67a53813102e549e1b7f3ed5"><span class="icon"></span>3647305</a></td><td><code>Some cleanup and adding more category information to particular sets.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=f810aa668c8d0c74ec861e9ab307bedc7777036a"><span class="icon"></span>f810aa6</a></td><td><code>Reworking the category of manifolds.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=375ff46221b7dcc101536774664b4bb935f99fba"><span class="icon"></span>375ff46</a></td><td><code>Fixing doctest failures and letting a few other rings know they are metric spaces.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=8b851a05c2569a55063539bbc8a19937851f4c93"><span class="icon"></span>8b851a0</a></td><td><code>Merge branch 'develop' into public/categories/topological_metric_spaces18175</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=f8f5b93028708eebb745a1cc92f3b775a760108b"><span class="icon"></span>f8f5b93</a></td><td><code>Fixing last remaining doctests.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=041a5d10259ca0ec083a313bd4ad1fe6cfb8d9c0"><span class="icon"></span>041a5d1</a></td><td><code>Adding padics to metric spaces and some cleanup.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=bfa0cdf3696eadd6c2307f96dd366916746d5366"><span class="icon"></span>bfa0cdf</a></td><td><code>One last doc tweak.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=d13c3688328149c08f4efbbcee91a8fd2978d91a"><span class="icon"></span>d13c368</a></td><td><code>Fixing doc of metric spaces.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=2605c0b887345dfc72a14936f5f0e267b8730f2e"><span class="icon"></span>2605c0b</a></td><td><code>Merge #18529 (Topological manifolds: basics) into #18175 (Implement categories for topological...)</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=6dec6d592a09e56921b9a761827309dd31ae2533"><span class="icon"></span>6dec6d5</a></td><td><code>Implement topological manifolds (basics, #18529) on the new categories for manifolds (#18175)</code>
</td></tr></table>
TickettscrimThu, 22 Oct 2015 17:18:50 GMT
https://trac.sagemath.org/ticket/18529#comment:23
https://trac.sagemath.org/ticket/18529#comment:23
<p>
I agree with Vincent here; we should be explicit in our names.
</p>
<p>
Although is there a reason why you just did not have a single <code>Manifold</code> function/class as a global entry point, which then could delegate out to <code>*Manifold</code> depending on some boolean input arguments? I feel that this would make the user interface easier and have a single collective point for general documentation. Another option I see would be to have a <code>manifolds</code> catalog which then would have things like <code>Topological</code>, <code>Differentiable</code>, etc.
</p>
TicketegourgoulhonThu, 22 Oct 2015 20:45:56 GMT
https://trac.sagemath.org/ticket/18529#comment:24
https://trac.sagemath.org/ticket/18529#comment:24
<p>
Yes, it is pretty clear that we should have the following renaming of *classes*:
</p>
<ul><li><code>TopManifold</code> > <code>TopologicalManifold</code>
</li><li><code>DiffManifold</code> > <code>DifferentiableManifold</code>
</li></ul><p>
Regarding the user interface, we could indeed have a *function* <code>Manifold</code> that looks like
</p>
<pre class="wiki">Manifold(dim, symbol, type='smooth', ...)
</pre><p>
with <code>'topological'</code>, <code>'differentiable'</code>, <code>'smooth'</code> as possible values for the parameter <code>type</code> (as well as <code>'analytic'</code>, etc. in the future). A question: should <code>'smooth'</code> be the default value?
</p>
<p>
Besides, I remember someone was complaining that <code>Manifold</code> was already used in SnapPy, cf. <a class="extlink" href="http://www.math.uic.edu/t3m/SnapPy/manifold.html"><span class="icon"></span>this page</a>, but is there any danger of name clash? i.e. is SnapPy used (or could be used in the future) from Sage?
</p>
TickettscrimFri, 23 Oct 2015 17:01:06 GMT
https://trac.sagemath.org/ticket/18529#comment:25
https://trac.sagemath.org/ticket/18529#comment:25
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:24" title="Comment 24">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Regarding the user interface, we could indeed have a *function* <code>Manifold</code> that looks like
</p>
<pre class="wiki">Manifold(dim, symbol, type='smooth', ...)
</pre><p>
with <code>'topological'</code>, <code>'differentiable'</code>, <code>'smooth'</code> as possible values for the parameter <code>type</code> (as well as <code>'analytic'</code>, etc. in the future). A question: should <code>'smooth'</code> be the default value?
</p>
</blockquote>
<p>
I don't know how easy it would be to make this a check on the atlas used to construct the manifold. I would make it select it automatically if possible, and then default to the most general setting of <code>'topological'</code>.
</p>
<blockquote class="citation">
<p>
Besides, I remember someone was complaining that <code>Manifold</code> was already used in SnapPy, cf. <a class="extlink" href="http://www.math.uic.edu/t3m/SnapPy/manifold.html"><span class="icon"></span>this page</a>, but is there any danger of name clash? i.e. is SnapPy used (or could be used in the future) from Sage?
</p>
</blockquote>
<p>
I would then add an additional argument of <code>implementation</code>, which would default to the native Sage implementation, if there does become a name clash and have the constructor delegate out to the SnapPy version when it exists in Sage (IDK if it is Sage now or not).
</p>
TicketegourgoulhonSat, 24 Oct 2015 13:35:35 GMT
https://trac.sagemath.org/ticket/18529#comment:26
https://trac.sagemath.org/ticket/18529#comment:26
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:25" title="Comment 25">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I don't know how easy it would be to make this a check on the atlas used to construct the manifold. I would make it select it automatically if possible, and then default to the most general setting of <code>'topological'</code>.
</p>
</blockquote>
<p>
Actually in the current setting, the atlas is not specified at the construction of the manifold: charts and transition maps are introduced on the fly, at any step in the user workflow. Therefore the type of manifold has to be declared explicitly by the user. It is my impression (but I may be biased) that one speaks about a "manifold" without any qualifier, one implicitely means a "real smooth manifold". Therefore I would set the default arguments of the <code>Manifold</code> function to <code>field='real'</code> and <code>type='smooth'</code>.
</p>
<blockquote class="citation">
<p>
I would then add an additional argument of <code>implementation</code>, which would default to the native Sage implementation, if there does become a name clash and have the constructor delegate out to the SnapPy version when it exists in Sage (IDK if it is Sage now or not).
</p>
</blockquote>
<p>
Good idea!
</p>
TickettscrimSun, 25 Oct 2015 22:26:34 GMT
https://trac.sagemath.org/ticket/18529#comment:27
https://trac.sagemath.org/ticket/18529#comment:27
<p>
Some things from looking over the current structure:
</p>
<ul><li>I would separate out parts of the subset class that applies to <code>Top(ological)Manifold</code> and <code>TopManifoldSubset</code> into an ABC (abstract base class) so you don't have to do things like <code>self is manifold</code>.
</li></ul><ul><li>Maybe put the subsets into the <code>Subobjects</code> class and maybe consider not making it a facade?
</li></ul><ul><li>Should manifolds and their subsets be <code>UniqueRepresentation</code>? I understand that the name as the only input means they are essentially treated as variables, but it means you have to do things like <code>TopManifold._clear_cache_()</code> (which may be needed not just necessarily in doctests if one does this before the gc comes around).
</li></ul><ul><li>Related to the above, we want parents to be hashable and have a good equality. I know that by making it a <code>UniqueRepresentation</code>, many of these issues are solved. However the manifolds are essentially mutable, but by doing a <code>__hash__</code> which only depends upon the name(s) and dimension means this is okay (<code>__eq__</code> defaults to check by <code>is</code>). This would alleviate the need for <code>UniqueRepresentation</code>.
</li></ul><ul><li>In the tests for <code>_element_constructor_</code>, you shouldn't call it explicitly but via <code>X(...)</code>, which also checks that it is working correctly with the coercion framework.
</li></ul><ul><li>I don't quite agree with checking <code>self._field == RR</code> for real charts as the precision should not matter. I would check <code>isinstance(self._field, RealField)</code> instead. Granted, we probably should have a function that checks against all known real field implementations...
</li></ul><ul><li>In a similar vein, I don't like the input of <code>'real'</code> to <code>RR</code> (and <code>'complex'</code> to <code>CC</code>). I would make the user be explicit about what they want unless you are doing to do some special handling to make this pass special arguments to an underlying <code>SR</code> implementation.
</li></ul>
TicketegourgoulhonMon, 26 Oct 2015 16:02:16 GMT
https://trac.sagemath.org/ticket/18529#comment:28
https://trac.sagemath.org/ticket/18529#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:27" title="Comment 27">tscrim</a>:
</p>
<p>
Thanks for your comments/suggestions.
</p>
<blockquote class="citation">
<p>
Some things from looking over the current structure:
</p>
<ul><li>I would separate out parts of the subset class that applies to <code>Top(ological)Manifold</code> and <code>TopManifoldSubset</code> into an ABC (abstract base class) so you don't have to do things like <code>self is manifold</code>.
</li></ul></blockquote>
<p>
I am not sure an ABC would help here: this would make a clear distinction between the manifold and strict subsets of it (thus avoiding the very few tests <code>self is self._manifold</code>), but on the other hand, we need open strict subsets to be in the class <code>TopologicalManifolds</code>.
</p>
<blockquote class="citation">
<ul><li>Maybe put the subsets into the <code>Subobjects</code> class and maybe consider not making it a facade?
</li></ul></blockquote>
<p>
I had the impression that the facade feature is exactly what we need, since we can have the same point created from different facade parents by means of different charts: if <code>U</code> and <code>V</code> are two overelapping open subsets of manifold <code>M</code>, it may happen that the same point of <code>M</code> is declared in two different ways:
</p>
<pre class="wiki"> p = U((x,y), chart=C1)
q = V((u,v), chart=C2)
</pre><p>
Thanks to the facade mecanism, <code>p</code> and <code>q</code> will have the same parent: <code>M</code> and then it is possible to check whether <code>p == q</code>; this will hold if the coordinates <code>(x,y)</code> in chart <code>C1</code> correspond to coordinates <code>(u,v)</code> in chart <code>C2</code>. Is there something bad in using facade parents?
</p>
<blockquote class="citation">
<ul><li>Should manifolds and their subsets be <code>UniqueRepresentation</code>? I understand that the name as the only input means they are essentially treated as variables, but it means you have to do things like <code>TopManifold._clear_cache_()</code> (which may be needed not just necessarily in doctests if one does this before the gc comes around).
</li></ul></blockquote>
<p>
Actually, I don't see any use case (except for confusing the reader!) where one would define two different manifolds with the same name and same dimension over the same topological field. So <code>UniqueRepresentation</code> seems quite appropriate here. Moreover, I thought this is Sage's credo to have parents be <code>UniqueRepresentation</code>, cf. the phrase <em>You are encouraged to make your parent "unique"</em> in the tutorial <a class="extlink" href="http://doc.sagemath.org/html/en/thematic_tutorials/coercion_and_categories.html#coercionandcategories"><span class="icon"></span>How to implement new algebraic structures in Sage</a>. Also, most parents in <code>src/sage/geometry</code> (in particular the hyperbolic plane) have <code>UniqueRepresentation</code>.
</p>
<blockquote class="citation">
<ul><li>Related to the above, we want parents to be hashable and have a good equality. I know that by making it a <code>UniqueRepresentation</code>, many of these issues are solved.
</li></ul></blockquote>
<p>
Indeed!
</p>
<blockquote class="citation">
<p>
However the manifolds are essentially mutable, but by doing a <code>__hash__</code> which only depends upon the name(s) and dimension means this is okay (<code>__eq__</code> defaults to check by <code>is</code>). This would alleviate the need for <code>UniqueRepresentation</code>.
</p>
</blockquote>
<p>
Why should we avoid <code>UniqueRepresentation</code>? It's true that manifolds are mutable as Python objects, since the main idea is to add charts (and vector frames for differentiable manifolds) "on the fly" (for, in a concrete calculation, charts are often defined from previous ones by some transition map that might depend on some computational result and therefore it is impossible to introduce all charts at the instantiation of the manifold object). But this mutability is "secondary": the primary attributes of a manifold being its dimension, its name and its base field. With respect to these, the manifold is immutable. The need for <code>_clear_cache_()</code> in some doctests seems a very small annoyment, with respect to the benefit of <code>UniqueRepresentation</code>, doesn't it ? (I am afraid I have not understood the nondoctest case involving the garbage collector that you mentioned)
</p>
<blockquote class="citation">
<ul><li>In the tests for <code>_element_constructor_</code>, you shouldn't call it explicitly but via <code>X(...)</code>, which also checks that it is working correctly with the coercion framework.
</li></ul></blockquote>
<p>
OK, I will change this.
</p>
<blockquote class="citation">
<ul><li>I don't quite agree with checking <code>self._field == RR</code> for real charts as the precision should not matter. I would check <code>isinstance(self._field, RealField)</code> instead.
</li></ul></blockquote>
<p>
Yes, I agree. I was also not satisfied with this.
</p>
<blockquote class="citation">
<p>
Granted, we probably should have a function that checks against all known real field implementations...
</p>
</blockquote>
<blockquote class="citation">
<ul><li>In a similar vein, I don't like the input of <code>'real'</code> to <code>RR</code> (and <code>'complex'</code> to <code>CC</code>). I would make the user be explicit about what they want unless you are doing to do some special handling to make this pass special arguments to an underlying <code>SR</code> implementation.
</li></ul></blockquote>
<p>
Yes, I agree too.
</p>
TicketegourgoulhonTue, 27 Oct 2015 13:29:07 GMT
https://trac.sagemath.org/ticket/18529#comment:29
https://trac.sagemath.org/ticket/18529#comment:29
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:28" title="Comment 28">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:27" title="Comment 27">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>I would separate out parts of the subset class that applies to <code>Top(ological)Manifold</code> and <code>TopManifoldSubset</code> into an ABC (abstract base class) so you don't have to do things like <code>self is manifold</code>.
</li></ul></blockquote>
<p>
I am not sure an ABC would help here: this would make a clear distinction between the manifold and strict subsets of it (thus avoiding the very few tests <code>self is self._manifold</code>), but on the other hand, we need open strict subsets to be in the class <code>TopologicalManifolds</code>.
</p>
</blockquote>
<p>
I gave a second thought to this: are you thinking about something like
</p>
<pre class="wiki"> The_ABC
/ \
TopologicalManifoldStrictSubset TopologicalManifold
\ /
TopologicalManifoldStrictOpenSubset
</pre><p>
with the methods superset(), intersection() and union() being implemented in each of the classes <code>TopologicalManifoldStrictSubset</code> and <code>TopologicalManifold</code> ?
</p>
TickettscrimWed, 28 Oct 2015 04:45:40 GMT
https://trac.sagemath.org/ticket/18529#comment:30
https://trac.sagemath.org/ticket/18529#comment:30
<p>
In a short word, yes, that is correct. I will respond in more detail tomorrow morning when I wake up.
</p>
TickettscrimWed, 28 Oct 2015 15:04:39 GMT
https://trac.sagemath.org/ticket/18529#comment:31
https://trac.sagemath.org/ticket/18529#comment:31
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:28" title="Comment 28">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:27" title="Comment 27">tscrim</a>:
</p>
<p>
Thanks for your comments/suggestions.
</p>
<blockquote class="citation">
<p>
Some things from looking over the current structure:
</p>
<ul><li>I would separate out parts of the subset class that applies to <code>Top(ological)Manifold</code> and <code>TopManifoldSubset</code> into an ABC (abstract base class) so you don't have to do things like <code>self is manifold</code>.
</li></ul></blockquote>
<p>
I am not sure an ABC would help here: this would make a clear distinction between the manifold and strict subsets of it (thus avoiding the very few tests <code>self is self._manifold</code>), but on the other hand, we need open strict subsets to be in the class <code>TopologicalManifolds</code>.
</p>
</blockquote>
<p>
The diagram you have below is what I was thinking (at least the first 2 levels). This gives better separation of concerns and better distinguishes the two classes.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Maybe put the subsets into the <code>Subobjects</code> class and maybe consider not making it a facade?
</li></ul></blockquote>
<p>
I had the impression that the facade feature is exactly what we need, since we can have the same point created from different facade parents by means of different charts: if <code>U</code> and <code>V</code> are two overelapping open subsets of manifold <code>M</code>, it may happen that the same point of <code>M</code> is declared in two different ways:
</p>
<pre class="wiki"> p = U((x,y), chart=C1)
q = V((u,v), chart=C2)
</pre><p>
Thanks to the facade mecanism, <code>p</code> and <code>q</code> will have the same parent: <code>M</code> and then it is possible to check whether <code>p == q</code>; this will hold if the coordinates <code>(x,y)</code> in chart <code>C1</code> correspond to coordinates <code>(u,v)</code> in chart <code>C2</code>. Is there something bad in using facade parents?
</p>
</blockquote>
<p>
It depends on what you want the points associated with. If they are to be considered proper subsets of the manifold, where you want to do operations, then the point should know that it belongs to the subset. If it is really just reflecting a particular chart, then you probably should reconsider your entire class hierarchy because I think it shouldn't quack like a manifold in that case.
</p>
<p>
It really comes down to what you're doing mostly in the code. If you'd be doing a lot of conversions between points thought of as being in the subset and as in the manifold, then a facade is probably an okay way to go. If you care about associating the point with a particular subset, then it should not be a facade.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Should manifolds and their subsets be <code>UniqueRepresentation</code>? I understand that the name as the only input means they are essentially treated as variables, but it means you have to do things like <code>TopManifold._clear_cache_()</code> (which may be needed not just necessarily in doctests if one does this before the gc comes around).
</li></ul></blockquote>
<p>
Actually, I don't see any use case (except for confusing the reader!) where one would define two different manifolds with the same name and same dimension over the same topological field. So <code>UniqueRepresentation</code> seems quite appropriate here. Moreover, I thought this is Sage's credo to have parents be <code>UniqueRepresentation</code>, cf. the phrase <em>You are encouraged to make your parent "unique"</em> in the tutorial <a class="extlink" href="http://doc.sagemath.org/html/en/thematic_tutorials/coercion_and_categories.html#coercionandcategories"><span class="icon"></span>How to implement new algebraic structures in Sage</a>. Also, most parents in <code>src/sage/geometry</code> (in particular the hyperbolic plane) have <code>UniqueRepresentation</code>.
</p>
</blockquote>
<p>
The hyperbolic plane works well because it is uniquely defined. Also not all parents are <code>UniqueRepresentations</code>, the reason why we do this is because of the coercion framework and speed. Identity checks are typically much faster than equality checks.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
However the manifolds are essentially mutable, but by doing a <code>__hash__</code> which only depends upon the name(s) and dimension means this is okay (<code>__eq__</code> defaults to check by <code>is</code>). This would alleviate the need for <code>UniqueRepresentation</code>.
</p>
</blockquote>
<p>
Why should we avoid <code>UniqueRepresentation</code>? It's true that manifolds are mutable as Python objects, since the main idea is to add charts (and vector frames for differentiable manifolds) "on the fly" (for, in a concrete calculation, charts are often defined from previous ones by some transition map that might depend on some computational result and therefore it is impossible to introduce all charts at the instantiation of the manifold object). But this mutability is "secondary": the primary attributes of a manifold being its dimension, its name and its base field. With respect to these, the manifold is immutable. The need for <code>_clear_cache_()</code> in some doctests seems a very small annoyment, with respect to the benefit of <code>UniqueRepresentation</code>, doesn't it ? (I am afraid I have not understood the nondoctest case involving the garbage collector that you mentioned)
</p>
</blockquote>
<p>
This comes down to the manifold not being welldefined by its dimension, name, and base field. It also needs to be defined by its chart. I understand that giving two distinct manifolds the same name is bad mathematically, and you are reflecting that in your code. However what if I create a manifold <code>M</code>, then decide I want to create a new (and different) manifold <code>M</code> of the same dimension over the same base field? When I try to do this naïvely, I get all of the previous chart information much to my surprise (the garbage collector this was more about temporary objects).
</p>
TicketegourgoulhonWed, 28 Oct 2015 21:00:19 GMT
https://trac.sagemath.org/ticket/18529#comment:32
https://trac.sagemath.org/ticket/18529#comment:32
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:31" title="Comment 31">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It really comes down to what you're doing mostly in the code. If you'd be doing a lot of conversions between points thought of as being in the subset and as in the manifold, then a facade is probably an okay way to go. If you care about associating the point with a particular subset, then it should not be a facade.
</p>
</blockquote>
<p>
I think we are precisely in the first case. In particular we do not care about associating the point with a particular subset (since many overlapping subsets can be introduced that contain the point and none of them is to be privileged with respect to the point). So I would say that we should use the facade.
</p>
<blockquote class="citation">
<p>
The hyperbolic plane works well because it is uniquely defined. Also not all parents are <code>UniqueRepresentations</code>, the reason why we do this is because of the coercion framework and speed. Identity checks are typically much faster than equality checks.
</p>
</blockquote>
<p>
Thanks for these explanations.
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<p>
However the manifolds are essentially mutable, but by doing a <code>__hash__</code> which only depends upon the name(s) and dimension means this is okay (<code>__eq__</code> defaults to check by <code>is</code>). This would alleviate the need for <code>UniqueRepresentation</code>.
</p>
</blockquote>
<p>
Why should we avoid <code>UniqueRepresentation</code>? It's true that manifolds are mutable as Python objects, since the main idea is to add charts (and vector frames for differentiable manifolds) "on the fly" (for, in a concrete calculation, charts are often defined from previous ones by some transition map that might depend on some computational result and therefore it is impossible to introduce all charts at the instantiation of the manifold object). But this mutability is "secondary": the primary attributes of a manifold being its dimension, its name and its base field. With respect to these, the manifold is immutable. The need for <code>_clear_cache_()</code> in some doctests seems a very small annoyment, with respect to the benefit of <code>UniqueRepresentation</code>, doesn't it ? (I am afraid I have not understood the nondoctest case involving the garbage collector that you mentioned)
</p>
</blockquote>
<p>
This comes down to the manifold not being welldefined by its dimension, name, and base field. It also needs to be defined by its chart.
</p>
</blockquote>
<p>
Indeed. More precisely, a manifold is defined by its maximal atlas. But there is no way to represent the latter in Sage, nor in any computer system. Accordingly, we cannot check the equality of two manifolds. This is why equality by id seems reasonable here.
</p>
<blockquote class="citation">
<p>
I understand that giving two distinct manifolds the same name is bad mathematically, and you are reflecting that in your code. However what if I create a manifold <code>M</code>, then decide I want to create a new (and different) manifold <code>M</code> of the same dimension over the same base field? When I try to do this naïvely, I get all of the previous chart information much to my surprise (the garbage collector this was more about temporary objects).
</p>
</blockquote>
<p>
You are right: a user may want to restart some computation by creating a manifold with the same name as that used previously. If he is doing so in the same session, the <code>UniqueRepresentation</code> behavior will return the previously created object. We cannot demand that the user runs <code>_clear_cache_()</code> before creating the manifold...
</p>
TickettscrimSat, 31 Oct 2015 15:34:19 GMT
https://trac.sagemath.org/ticket/18529#comment:33
https://trac.sagemath.org/ticket/18529#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:32" title="Comment 32">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:31" title="Comment 31">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It really comes down to what you're doing mostly in the code. If you'd be doing a lot of conversions between points thought of as being in the subset and as in the manifold, then a facade is probably an okay way to go. If you care about associating the point with a particular subset, then it should not be a facade.
</p>
</blockquote>
<p>
I think we are precisely in the first case. In particular we do not care about associating the point with a particular subset (since many overlapping subsets can be introduced that contain the point and none of them is to be privileged with respect to the point). So I would say that we should use the facade.
</p>
</blockquote>
<p>
Then we go with that.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
This comes down to the manifold not being welldefined by its dimension, name, and base field. It also needs to be defined by its chart.
</p>
</blockquote>
<p>
Indeed. More precisely, a manifold is defined by its maximal atlas. But there is no way to represent the latter in Sage, nor in any computer system. Accordingly, we cannot check the equality of two manifolds. This is why equality by id seems reasonable here.
</p>
</blockquote>
<p>
Then we are in agreement: equality by id.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
I understand that giving two distinct manifolds the same name is bad mathematically, and you are reflecting that in your code. However what if I create a manifold <code>M</code>, then decide I want to create a new (and different) manifold <code>M</code> of the same dimension over the same base field? When I try to do this naïvely, I get all of the previous chart information much to my surprise (the garbage collector this was more about temporary objects).
</p>
</blockquote>
<p>
You are right: a user may want to restart some computation by creating a manifold with the same name as that used previously. If he is doing so in the same session, the <code>UniqueRepresentation</code> behavior will return the previously created object. We cannot demand that the user runs <code>_clear_cache_()</code> before creating the manifold...
</p>
</blockquote>
<p>
As I outlined above, it should be easy enough to remove the <code>UniqueRepresentation</code> behavior and still retain the desirable behaviors such as hashability.
</p>
TicketegourgoulhonSat, 31 Oct 2015 15:39:14 GMT
https://trac.sagemath.org/ticket/18529#comment:34
https://trac.sagemath.org/ticket/18529#comment:34
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:33" title="Comment 33">tscrim</a>:
</p>
<blockquote class="citation">
<p>
As I outlined above, it should be easy enough to remove the <code>UniqueRepresentation</code> behavior and still retain the desirable behaviors such as hashability.
</p>
</blockquote>
<p>
I am on it and should push a new commit soon.
</p>
TickettscrimSat, 31 Oct 2015 15:44:24 GMT
https://trac.sagemath.org/ticket/18529#comment:35
https://trac.sagemath.org/ticket/18529#comment:35
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:34" title="Comment 34">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:33" title="Comment 33">tscrim</a>:
</p>
<blockquote class="citation">
<p>
As I outlined above, it should be easy enough to remove the <code>UniqueRepresentation</code> behavior and still retain the desirable behaviors such as hashability.
</p>
</blockquote>
<p>
I am on it and should push a new commit soon.
</p>
</blockquote>
<p>
Thanks. Also thank you for all your work on this.
</p>
TicketgitSun, 01 Nov 2015 20:26:39 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:36
https://trac.sagemath.org/ticket/18529#comment:36
<ul>
<li><strong>commit</strong>
changed from <em>6dec6d592a09e56921b9a761827309dd31ae2533</em> to <em>f342e03e7008831c4789b94b03674c1a0cbbf3a6</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=f582241f35ad8d843020580858347d763784715e"><span class="icon"></span>f582241</a></td><td><code>Introduce function Manifold() as the global entry point to construct any type of manifold.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=f342e03e7008831c4789b94b03674c1a0cbbf3a6"><span class="icon"></span>f342e03</a></td><td><code>Remove UniqueRepresentation from topological manifolds, subsets and charts.</code>
</td></tr></table>
TicketegourgoulhonSun, 01 Nov 2015 21:03:39 GMT
https://trac.sagemath.org/ticket/18529#comment:37
https://trac.sagemath.org/ticket/18529#comment:37
<p>
The above commit addresses some of the comments by Travis and Vincent:
</p>
<ul><li>The class <code>TopManifold</code> has been renamed <code>TopologicalManifold</code> and the class <code>TopManifoldSubset</code> has been renamed <code>TopologicalManifoldSubset</code>.
</li><li>A function <code>Manifold</code> has been introduced as the unique entry point to construct manifolds (this is the only piece that is imported in the global namespace, as a lazy import).
</li><li>Tests of <code>_element_constructor_</code> are performed by call to the parent.
</li><li>The classes <code>TopologicalManifold</code>, <code>TopologicalManifoldSubset</code>, <code>Chart</code> and <code>RealChart</code> do not longer inherit from <code>UniqueRepresentation</code>. Consequently, they have been provided with functions <code>__hash__</code>, <code>__eq__</code>, <code>__ne__</code> and <code>__reduce__</code> (the latter turned out to be necessary for correct pickling). In addition, the class <code>Chart</code> has been endowed with <code>__getstate__</code> and <code>__setstate__</code>.
</li></ul><p>
It seemed necessary to implement a proper <code>__eq__</code>, i.e. not to have equality by id, since we cannot afford equality by id when relaxing the unique representation, otherwise the pickling test <code>loads(dumps(M)) == M</code> would failed, since obviously <code>id(loads(dumps(M))</code> differs from <code>id(M)</code>.
</p>
<p>
Other issues mentionned by Travis have not been addressed yet:
</p>
<ul><li>need for an ABC for manifolds and their subsets, introduce a specific class for open subsets (currently they are dealt by class <code>TopologicalManifold</code>, with the attribute <code>self._manifold</code> representing the ambient manifold)
</li><li>treatment of real and complex fields as base fields (i.e. avoiding to default to the finite precision representations <code>RR</code> and <code>CC</code>)
</li></ul>
TickettscrimMon, 02 Nov 2015 04:21:40 GMT
https://trac.sagemath.org/ticket/18529#comment:38
https://trac.sagemath.org/ticket/18529#comment:38
<p>
Oh yea...pickling. Uggg...
</p>
<p>
I worry that the current equality check is too general. Perhaps also for equality add a check to see if the current atlases are equal (or one is a subset of the other), which in turn would have to check equality on charts? Overall it is not pretty, but I would hope one is not checking equality of manifolds too often (and we can somewhat short circuit this by first checking <code>self is other</code> [which is also a general python recommendation]).
</p>
TicketegourgoulhonMon, 02 Nov 2015 08:35:55 GMT
https://trac.sagemath.org/ticket/18529#comment:39
https://trac.sagemath.org/ticket/18529#comment:39
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:38" title="Comment 38">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Oh yea...pickling. Uggg...
</p>
<p>
I worry that the current equality check is too general. Perhaps also for equality add a check to see if the current atlases are equal (or one is a subset of the other), which in turn would have to check equality on charts? Overall it is not pretty, but I would hope one is not checking equality of manifolds too often
</p>
</blockquote>
<p>
Unfortunately yes: charts are used as dictionary keys for the coordinate expressions of scalar functions defined on the manifold (these functions will appear massively as the components of tensor fields), therefore one checks often equality of charts and the latter starts by checking the equality of manifolds on which they are defined. By the way, if we introduce the check of atlases in the manifold equality, there is a danger of endless loop: to compare charts, one first compare their domains, i.e. manifolds... To circumvent this, maybe we should introduce two comparison operators: a fast <code>__eq__</code>, which cheks only the name, dimension and base field and a more mathematically meaningful <code>is_isomorphic_to</code>, which checks in addition the atlases.
</p>
TicketegourgoulhonTue, 03 Nov 2015 10:31:02 GMT
https://trac.sagemath.org/ticket/18529#comment:40
https://trac.sagemath.org/ticket/18529#comment:40
<p>
I've already noticed some important loss of performance due to the equality not being by id; I am afraid that for tensor computations this will become much worse. Moreover, a sophisticated equality check (either with <code>__eq__</code> or <code>is_isomorphic_to</code>) seems difficult to acheive: checking the equality of the userdefined atlases is definitevely not sufficient to assert the mathematical equality of two manifolds; one should compare the maximal atlases instead, which is impossible. For example, if one first construct S<sup>2</sup> with an atlas of two stereographic charts and then another S<sup>2</sup> with an atlas of two polar charts, the two atlases differ, while both objects represent the same manifold. There is also the issue of endless loop mentioned in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:39" title="Comment 39">comment:39</a>.
</p>
<p>
For the above reasons, I am considering to revert to the <code>UniqueRepresentation</code> for manifolds. To solve the issue of the redefinition by the end user disccused in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:32" title="Comment 32">comment:32</a>, we could have some handling of the cache in the function <code>Manifold</code>, so that
</p>
<pre class="wiki"> M1 = Manifold(2, 'M')
M2 = Manifold(2, 'M')
</pre><p>
will construct two different objects. What do you think?
</p>
TickettscrimTue, 03 Nov 2015 16:22:30 GMT
https://trac.sagemath.org/ticket/18529#comment:41
https://trac.sagemath.org/ticket/18529#comment:41
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:40" title="Comment 40">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
I've already noticed some important loss of performance due to the equality not being by id; I am afraid that for tensor computations this will become much worse.
</p>
</blockquote>
<p>
Did you have as the first lines of the <code>__eq__</code> being
</p>
<pre class="wiki">if self is other:
return True
</pre><p>
since "most" equality checks should be by identity. Granted, we do lose some speed because this is a python check, as opposed to a cython check.
</p>
<blockquote class="citation">
<p>
Moreover, a sophisticated equality check (either with <code>__eq__</code> or <code>is_isomorphic_to</code>) seems difficult to acheive: checking the equality of the userdefined atlases is definitevely not sufficient to assert the mathematical equality of two manifolds; one should compare the maximal atlases instead, which is impossible. For example, if one first construct S<sup>2</sup> with an atlas of two stereographic charts and then another S<sup>2</sup> with an atlas of two polar charts, the two atlases differ, while both objects represent the same manifold.
</p>
</blockquote>
<p>
<code>__eq__</code> does not have to represent mathematical equality; IIRC we already have examples of this in Sage, both when <code>__eq__</code> is weaker and stronger than mathematical equality (which often times people wish it could be isomorphism). So we are okay with making <code>__eq__</code> not reflect the mathematical equality.
</p>
<blockquote class="citation">
<p>
There is also the issue of endless loop mentioned in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:39" title="Comment 39">comment:39</a>.
</p>
</blockquote>
<p>
The design reflects the mathematics as charts are morphism from <strong>R</strong><sup>n</sup> > <em>M</em> and that the manifold <em>M</em> is an abstract set of points. My proposal goes around the mathematical definition of a manifold as a set of points by saying the manifold <em>M</em> is defined by its charts. So this approach is definitely bad.
</p>
<blockquote class="citation">
<p>
For the above reasons, I am considering to revert to the <code>UniqueRepresentation</code> for manifolds. To solve the issue of the redefinition by the end user disccused in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:32" title="Comment 32">comment:32</a>, we could have some handling of the cache in the function <code>Manifold</code>, so that
</p>
<pre class="wiki"> M1 = Manifold(2, 'M')
M2 = Manifold(2, 'M')
</pre><p>
will construct two different objects. What do you think?
</p>
</blockquote>
<p>
I worry that this will cause subtle issues with pickling. For example, if we clear the cache when creating <code>M2</code>, then <code>loads(dumps(M1))</code> will probably not equal <code>M1</code>, but instead equal <code>M2</code>. I haven't checked if this actually happens.
</p>
<p>
There are a few different options that I see at this point.
</p>
<ul><li>Drop pickling <code>==</code> support and use <code>sage.misc.fast_methods.WithEqualityById</code> (this will get your speed back). However we could get close by implementing pickling as saving the current atlas.
</li><li>Go back to <code>UniqueRepresentation</code>, but also have it additionally keyed by the time it was created:
<pre class="wiki">sage: import time
sage: time.time()
1446566813.121567
</pre>You could do this by having
<pre class="wiki">@staticmethod
def __classcall__(cls, dim, name, latex, R, time_key=None):
if time_key is None:
from time import time
time_key = time()
return super(TopologicalManifold, cls).__classcall__(cls, dim, latex, R, time_key=time_key)
def __init__(self, dim, name, latex, R, time_key):
# Just ignore the time_key input
</pre>(or doing it via a <code>UniqueFactory</code>).
</li><li>Warn the user that they can only create one manifold of a given (latex) name of a given dimension over a given field.
</li><li>Figure out some other way to better uniquely specify a manifold.
</li><li>Ask someone else, like Jeroen, Simon, Volker, and/or sagedevel, for other options.
</li></ul><p>
Personally I would go for first one since pickling within a session is, I believe, usually not needed/used.
</p>
TicketgitTue, 03 Nov 2015 23:30:41 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:42
https://trac.sagemath.org/ticket/18529#comment:42
<ul>
<li><strong>commit</strong>
changed from <em>f342e03e7008831c4789b94b03674c1a0cbbf3a6</em> to <em>902908b41a95d3455bfcc497997ad2054c530a96</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=902908b41a95d3455bfcc497997ad2054c530a96"><span class="icon"></span>902908b</a></td><td><code>Revert to UniqueRepresentation for topological manifolds and charts, with the possibility to reuse manifold names.</code>
</td></tr></table>
TicketegourgoulhonTue, 03 Nov 2015 23:52:42 GMT
https://trac.sagemath.org/ticket/18529#comment:43
https://trac.sagemath.org/ticket/18529#comment:43
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:41" title="Comment 41">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:40" title="Comment 40">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
I've already noticed some important loss of performance due to the equality not being by id; I am afraid that for tensor computations this will become much worse.
</p>
</blockquote>
<p>
Did you have as the first lines of the <code>__eq__</code> being
</p>
<pre class="wiki">if self is other:
return True
</pre></blockquote>
<p>
Actually no (except for points).
</p>
<blockquote class="citation">
<p>
since "most" equality checks should be by identity.
</p>
</blockquote>
<p>
Well, as soon as we have more than two charts on a manifold, this is no longer true.
</p>
<blockquote class="citation">
<p>
Granted, we do lose some speed because this is a python check, as opposed to a cython check.
</p>
</blockquote>
<blockquote class="citation">
<p>
<code>__eq__</code> does not have to represent mathematical equality; IIRC we already have examples of this in Sage, both when <code>__eq__</code> is weaker and stronger than mathematical equality (which often times people wish it could be isomorphism). So we are okay with making <code>__eq__</code> not reflect the mathematical equality.
</p>
</blockquote>
<p>
OK.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
For the above reasons, I am considering to revert to the <code>UniqueRepresentation</code> for manifolds. To solve the issue of the redefinition by the end user disccused in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:32" title="Comment 32">comment:32</a>, we could have some handling of the cache in the function <code>Manifold</code>, so that
</p>
<pre class="wiki"> M1 = Manifold(2, 'M')
M2 = Manifold(2, 'M')
</pre><p>
will construct two different objects. What do you think?
</p>
</blockquote>
<p>
I worry that this will cause subtle issues with pickling. For example, if we clear the cache when creating <code>M2</code>, then <code>loads(dumps(M1))</code> will probably not equal <code>M1</code>, but instead equal <code>M2</code>. I haven't checked if this actually happens.
</p>
</blockquote>
<p>
Thanks for pointing this, I think you are right.
</p>
<blockquote class="citation">
<p>
There are a few different options that I see at this point.
</p>
<ul><li>Drop pickling <code>==</code> support and use <code>sage.misc.fast_methods.WithEqualityById</code> (this will get your speed back). However we could get close by implementing pickling as saving the current atlas.
</li></ul></blockquote>
<p>
I am quite reluctant to drop pickling <code>==</code> support, although, as you say, pickling within a session is usually not needed/used. This would break all <code>TestSuite()</code> tests, except of course if we run them with <code>skip='_test_pickling'</code>.
</p>
<blockquote class="citation">
<ul><li>Go back to <code>UniqueRepresentation</code>, but also have it additionally keyed by the time it was created:
<pre class="wiki">sage: import time
sage: time.time()
1446566813.121567
</pre>You could do this by having
<pre class="wiki">@staticmethod
def __classcall__(cls, dim, name, latex, R, time_key=None):
if time_key is None:
from time import time
time_key = time()
return super(TopologicalManifold, cls).__classcall__(cls, dim, latex, R, time_key=time_key)
def __init__(self, dim, name, latex, R, time_key):
# Just ignore the time_key input
</pre>(or doing it via a <code>UniqueFactory</code>).
</li></ul></blockquote>
<p>
I like very much this one; thanks a lot for suggesting it! I have implemented it in the above commit, with a small modification: I have not redefined <code>TopologicalManifold.__classcall__</code> (which is inherited from <code>UniqueRepresentation</code>), instead I have simply set the time tag in the function <code>Manifold</code>. Everything works well: see the rubric "Reusability of the manifold name" in the documentation of the function <code>Manifold</code>.
</p>
<blockquote class="citation">
<ul><li>Ask someone else, like Jeroen, Simon, Volker, and/or sagedevel, for other options.
</li></ul></blockquote>
<p>
Good idea, I will.
</p>
TicketegourgoulhonWed, 04 Nov 2015 07:26:53 GMTdescription changed
https://trac.sagemath.org/ticket/18529#comment:44
https://trac.sagemath.org/ticket/18529#comment:44
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=44">diff</a>)
</li>
</ul>
TicketnbruinThu, 05 Nov 2015 01:01:40 GMT
https://trac.sagemath.org/ticket/18529#comment:45
https://trac.sagemath.org/ticket/18529#comment:45
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:37" title="Comment 37">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
It seemed necessary to implement a proper <code>__eq__</code>, i.e. not to have equality by id, since we cannot afford equality by id when relaxing the unique representation, otherwise the pickling test <code>loads(dumps(M)) == M</code> would failed, since obviously <code>id(loads(dumps(M))</code> differs from <code>id(M)</code>.
</p>
</blockquote>
<p>
Can't you just drop that test? I don't think it's something that has to be true:
</p>
<pre class="wiki">sage: A=object()
sage: loads(dumps(A)) == A
False
</pre>
TicketegourgoulhonThu, 05 Nov 2015 09:34:58 GMT
https://trac.sagemath.org/ticket/18529#comment:46
https://trac.sagemath.org/ticket/18529#comment:46
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:45" title="Comment 45">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:37" title="Comment 37">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
It seemed necessary to implement a proper <code>__eq__</code>, i.e. not to have equality by id, since we cannot afford equality by id when relaxing the unique representation, otherwise the pickling test <code>loads(dumps(M)) == M</code> would failed, since obviously <code>id(loads(dumps(M))</code> differs from <code>id(M)</code>.
</p>
</blockquote>
<p>
Can't you just drop that test? I don't think it's something that has to be true:
</p>
</blockquote>
<p>
Well actually this is part of the <code>_test_pickling</code> method, as defined in <code>sage/structure/sage_object.pyx</code>. So shall we run the test suite as <code>TestSuite(M).run(skip='_test_pickling')</code> or shall we redefine <code>_test_pickling</code>?
</p>
TicketjdemeyerThu, 05 Nov 2015 11:10:57 GMT
https://trac.sagemath.org/ticket/18529#comment:47
https://trac.sagemath.org/ticket/18529#comment:47
<p>
There is no reason to write ugly stuff like
</p>
<pre class="wiki">self.__eq__(other)
</pre><p>
and
</p>
<pre class="wiki">foo.__hash__()
</pre><p>
The alternatives
</p>
<pre class="wiki">self == other
</pre><p>
and
</p>
<pre class="wiki">hash(foo)
</pre><p>
are easier to read and faster.
</p>
TicketjdemeyerThu, 05 Nov 2015 11:12:06 GMT
https://trac.sagemath.org/ticket/18529#comment:48
https://trac.sagemath.org/ticket/18529#comment:48
<p>
In think this also holds for <code>type(foo)</code> instead if <code>foo.__class__</code> but I'm not 100% sure if those are really equivalent.
</p>
TicketegourgoulhonThu, 05 Nov 2015 12:45:15 GMT
https://trac.sagemath.org/ticket/18529#comment:49
https://trac.sagemath.org/ticket/18529#comment:49
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:47" title="Comment 47">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
There is no reason to write ugly stuff like
</p>
<pre class="wiki">self.__eq__(other)
</pre><p>
and
</p>
<pre class="wiki">foo.__hash__()
</pre><p>
The alternatives
</p>
<pre class="wiki">self == other
</pre><p>
and
</p>
<pre class="wiki">hash(foo)
</pre><p>
are easier to read and faster.
</p>
</blockquote>
<p>
OK I will change this. Thanks.
</p>
TicketegourgoulhonThu, 05 Nov 2015 13:00:05 GMT
https://trac.sagemath.org/ticket/18529#comment:50
https://trac.sagemath.org/ticket/18529#comment:50
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:48" title="Comment 48">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
In think this also holds for <code>type(foo)</code> instead if <code>foo.__class__</code> but I'm not 100% sure if those are really equivalent.
</p>
</blockquote>
<p>
From <a class="extlink" href="http://stackoverflow.com/questions/1060499/differencebetweentypeobjandobjclass"><span class="icon"></span>this link</a> and <a class="extlink" href="http://stackoverflow.com/questions/10386166/pythonselfclassvstypeself"><span class="icon"></span>that one</a>, it seems that for Python newstyle classes (i.e. the only classes used in Sage), <code>type(foo)</code> is fully equivalent to <code>foo.__class__</code> and it is indeed best practice to use <code>type(foo)</code>.
</p>
TicketgitMon, 09 Nov 2015 10:16:52 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:51
https://trac.sagemath.org/ticket/18529#comment:51
<ul>
<li><strong>commit</strong>
changed from <em>902908b41a95d3455bfcc497997ad2054c530a96</em> to <em>252e616cc053a3b76ee563282222507cd78c9fb8</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=252e616cc053a3b76ee563282222507cd78c9fb8"><span class="icon"></span>252e616</a></td><td><code>Remove UniqueRepresentation, leaving only WithEqualityById, for topological manifolds and charts.</code>
</td></tr></table>
TicketegourgoulhonMon, 09 Nov 2015 10:46:07 GMTdescription changed
https://trac.sagemath.org/ticket/18529#comment:52
https://trac.sagemath.org/ticket/18529#comment:52
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=52">diff</a>)
</li>
</ul>
<p>
The above commit takes into account <a class="extlink" href="https://groups.google.com/forum/#!topic/sagedevel/Vzfj1haZHho"><span class="icon"></span>this discussion on sagedevel</a>, as well as the first recommendation of Travis in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:41" title="Comment 41">comment:41</a> : it removes <code>UniqueRepresentation</code> for manifolds and charts, leaving only <code>WithEqualityById</code>. Some methods <code>_test_pickling</code> have been introduced. They are weaker than <code>SageObject._test_pickling</code> in the sense that they do not demand <code>loads(dumps(M)) == M</code> (which equalitybyid forbids without any unique representation). However, these local <code>_test_pickling</code> methods perform non trivial tests: they guarantee that <code>loads(dumps(M))</code> proceeds without any error and they check the identity of some characteristics between the unpickled object and the original one. All the test suites are passed.
</p>
<p>
In addition the above commit takes into account the recommendation of Jeroen in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:49" title="Comment 49">comment:49</a>.
</p>
TicketegourgoulhonMon, 09 Nov 2015 14:08:51 GMT
https://trac.sagemath.org/ticket/18529#comment:53
https://trac.sagemath.org/ticket/18529#comment:53
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:27" title="Comment 27">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>I don't quite agree with checking <code>self._field == RR</code> for real charts as the precision should not matter. I would check <code>isinstance(self._field, RealField)</code> instead. Granted, we probably should have a function that checks against all known real field implementations...
</li></ul><ul><li>In a similar vein, I don't like the input of <code>'real'</code> to <code>RR</code> (and <code>'complex'</code> to <code>CC</code>). I would make the user be explicit about what they want unless you are doing to do some special handling to make this pass special arguments to an underlying <code>SR</code> implementation.
</li></ul></blockquote>
<p>
Actually, the precision is not used in the current setting, so neither <code>RR</code> nor <code>RealField</code> is really necessary. <code>RR</code> was used as a substitute for the true real field, which cannot be represented in the computer. Looking further, why not using Sage's <code>RealLazyField</code> for this? It seems that it is intended to be closer to the true <strong>R</strong>. One drawback is that we have (for the moment)
</p>
<pre class="wiki">sage: RLF in Fields().Topological()
False
</pre><p>
So <code>RLF</code> cannot be passed to the manifold constructor.
</p>
<p>
The same considerations apply of course to <code>ComplexLazyField</code>.
</p>
TickettscrimMon, 09 Nov 2015 14:12:15 GMT
https://trac.sagemath.org/ticket/18529#comment:54
https://trac.sagemath.org/ticket/18529#comment:54
<p>
What we need is a category or ABC of realizations of the real field that allows us to set things like the category across the board and give us ways to tell a manifold is over the real numbers. Although perhaps in this case we can just handle the strings <code>"real"</code> and <code>"complex"</code> and have them default to the category of <code>Manifolds(RR)</code> and <code>Manifolds(CC)</code>.
</p>
TicketegourgoulhonTue, 10 Nov 2015 06:52:46 GMT
https://trac.sagemath.org/ticket/18529#comment:55
https://trac.sagemath.org/ticket/18529#comment:55
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:54" title="Comment 54">tscrim</a>:
</p>
<blockquote class="citation">
<p>
What we need is a category or ABC of realizations of the real field that allows us to set things like the category across the board and give us ways to tell a manifold is over the real numbers. Although perhaps in this case we can just handle the strings <code>"real"</code> and <code>"complex"</code> and have them default to the category of <code>Manifolds(RR)</code> and <code>Manifolds(CC)</code>.
</p>
</blockquote>
<p>
Yes, for the time being, we could have two attributes in the manifold class:
</p>
<ul><li><code>_field_type</code>, a string with values <code>'real'</code>, <code>'complex'</code> or <code>'other'</code>, which is the thing that is checked against to know if we are dealing with e.g. a real manifold; for instance, to decide whether the charts are constructed in the subclass <code>RealChart</code> of the generic class <code>Chart</code>.
</li><li><code>_field</code>, which contains the field for the manifold category; it could default to <code>RR</code> for <code>_field_type == 'real'</code>.
</li></ul><p>
Do you agree?
</p>
TickettscrimThu, 12 Nov 2015 14:55:34 GMT
https://trac.sagemath.org/ticket/18529#comment:56
https://trac.sagemath.org/ticket/18529#comment:56
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:55" title="Comment 55">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:54" title="Comment 54">tscrim</a>:
</p>
<blockquote class="citation">
<p>
What we need is a category or ABC of realizations of the real field that allows us to set things like the category across the board and give us ways to tell a manifold is over the real numbers. Although perhaps in this case we can just handle the strings <code>"real"</code> and <code>"complex"</code> and have them default to the category of <code>Manifolds(RR)</code> and <code>Manifolds(CC)</code>.
</p>
</blockquote>
<p>
Yes, for the time being, we could have two attributes in the manifold class:
</p>
<ul><li><code>_field_type</code>, a string with values <code>'real'</code>, <code>'complex'</code> or <code>'other'</code>, which is the thing that is checked against to know if we are dealing with e.g. a real manifold; for instance, to decide whether the charts are constructed in the subclass <code>RealChart</code> of the generic class <code>Chart</code>.
</li><li><code>_field</code>, which contains the field for the manifold category; it could default to <code>RR</code> for <code>_field_type == 'real'</code>
</li></ul><p>
Do you agree?
</p>
</blockquote>
<p>
Yes that sounds like a good plan along with some simple type checking for default values of <code>_field_type</code> against <code>RealField</code> and <code>ComplexField</code> since (IMO) these are what most users will use for the reals and complexes.
</p>
TicketgitTue, 17 Nov 2015 13:45:56 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:57
https://trac.sagemath.org/ticket/18529#comment:57
<ul>
<li><strong>commit</strong>
changed from <em>252e616cc053a3b76ee563282222507cd78c9fb8</em> to <em>65186990b1106f89652107356b60faa048113915</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=65186990b1106f89652107356b60faa048113915"><span class="icon"></span>6518699</a></td><td><code>Introduce the attribute _field_type in class TopologicalManifold to check for real and complex manifolds.</code>
</td></tr></table>
TicketgitThu, 19 Nov 2015 19:54:17 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:58
https://trac.sagemath.org/ticket/18529#comment:58
<ul>
<li><strong>commit</strong>
changed from <em>65186990b1106f89652107356b60faa048113915</em> to <em>0b08b114e03c063bd2e500ac900fd843fb12673b</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=d8397c14ab92138aa0c4da4d77547a616f63ff88"><span class="icon"></span>d8397c1</a></td><td><code>Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=0b08b114e03c063bd2e500ac900fd843fb12673b"><span class="icon"></span>0b08b11</a></td><td><code>Some small tweaks as part of the review.</code>
</td></tr></table>
TickettscrimThu, 19 Nov 2015 19:57:27 GMT
https://trac.sagemath.org/ticket/18529#comment:59
https://trac.sagemath.org/ticket/18529#comment:59
<p>
I've done some small doc/review tweaks, but I was wondering if you were doing any changes to it, because I want to do some moderate refactoring to try and simplify the structure. In particular, you are not really using the subsets as facade parents as the points know what subset they belong to.
</p>
TickettscrimThu, 19 Nov 2015 19:57:49 GMTreviewer set
https://trac.sagemath.org/ticket/18529#comment:60
https://trac.sagemath.org/ticket/18529#comment:60
<ul>
<li><strong>reviewer</strong>
set to <em>Travis Scrimshaw</em>
</li>
</ul>
TicketegourgoulhonThu, 19 Nov 2015 22:31:54 GMT
https://trac.sagemath.org/ticket/18529#comment:61
https://trac.sagemath.org/ticket/18529#comment:61
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:59" title="Comment 59">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I've done some small doc/review tweaks,
</p>
</blockquote>
<p>
Thanks for these changes!
</p>
<blockquote class="citation">
<p>
but I was wondering if you were doing any changes to it,
</p>
</blockquote>
<p>
No, not at the moment: I've worked on the subsequent tickets to check if the introduced changes (in particular the removal of unique representation) propagate smoothly. So far, so good...
</p>
<blockquote class="citation">
<p>
because I want to do some moderate refactoring to try and simplify the structure. In particular, you are not really using the subsets as facade parents as the points know what subset they belong to.
</p>
</blockquote>
<p>
Well, it is true that points have an attribute called <code>_subset</code>, but this is a misnomer: <code>_creation_subset</code> would have been better. Indeed, from a pure mathematical point of view, a point of a manifold has no privileged subset attached to it. On the contrary, the point belongs to an infinite number of intersecting subsets. The attribute <code>_subset</code>, which is set to the facade parent at the point creation, is used only for fast check in the methods <code>TopologicalManifoldSubset.__contains__</code> and <code>TopologicalManifold.__contains__</code>. I think it can be suppressed, at the price of a small decrease in efficiency. Therefore, I still think that the facade mechanism is appropriate here: the creation subset (i.e. the facade parent) should not play any role: only the whole manifold matters. This is particularly true when dealing with tangent planes (ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/19092" title="enhancement: Differentiable manifolds: tangent spaces (closed: fixed)">#19092</a>): for a given point p, we do not want to have two tangent spaces, T_p M and T_p U with U open subset of M, do we?
</p>
TickettscrimFri, 20 Nov 2015 19:02:57 GMT
https://trac.sagemath.org/ticket/18529#comment:62
https://trac.sagemath.org/ticket/18529#comment:62
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:61" title="Comment 61">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:59" title="Comment 59">tscrim</a>:
</p>
<blockquote class="citation">
<p>
but I was wondering if you were doing any changes to it,
</p>
</blockquote>
<p>
No, not at the moment: I've worked on the subsequent tickets to check if the introduced changes (in particular the removal of unique representation) propagate smoothly. So far, so good...
</p>
</blockquote>
<p>
I will then start my refactoring. I will try to make every change as granular as possible in the commits so we can cherrypick changes if you don't necessarily agree with them.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
because I want to do some moderate refactoring to try and simplify the structure. In particular, you are not really using the subsets as facade parents as the points know what subset they belong to.
</p>
</blockquote>
<p>
Well, it is true that points have an attribute called <code>_subset</code>, but this is a misnomer: <code>_creation_subset</code> would have been better. Indeed, from a pure mathematical point of view, a point of a manifold has no privileged subset attached to it. On the contrary, the point belongs to an infinite number of intersecting subsets. The attribute <code>_subset</code>, which is set to the facade parent at the point creation, is used only for fast check in the methods <code>TopologicalManifoldSubset.__contains__</code> and <code>TopologicalManifold.__contains__</code>. I think it can be suppressed, at the price of a small decrease in efficiency. Therefore, I still think that the facade mechanism is appropriate here: the creation subset (i.e. the facade parent) should not play any role: only the whole manifold matters. This is particularly true when dealing with tangent planes (ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/19092" title="enhancement: Differentiable manifolds: tangent spaces (closed: fixed)">#19092</a>): for a given point p, we do not want to have two tangent spaces, T_p M and T_p U with U open subset of M, do we?
</p>
</blockquote>
<p>
At present, anytime an element is passed to the <code>_element_constructor_</code>, a new instance of that point is created. In a way, you are hacking the coercion/category framework by setting the parent of the point to be the manifold, so <code>M(p)</code> just shortcuts out to return <code>p</code>. The code tells me that points should be elements of a particular subset.
</p>
<p>
For the tangent spaces, you should coerce the point to the manifold in the distinguished chart as the input. You're going to have to do this anyways:
</p>
<pre class="wiki">sage: M = Manifold(2, 'M', field='real', type='topological')
sage: X.<x,y> = M.chart()
sage: U.<u,v> = M.chart()
sage: trans = X.transition_map(U, [x2,y2])
sage: p = M.point((0,0), X)
sage: p2 = M.point((2,2), U)
sage: p == p2
True
sage: p is p2
False
</pre><p>
How about this, let me make my changes and you can see what breaks or how much things slow down and we will use that to decide what we should do going forward. Does that sound reasonable to try? (In general, I would also argue that users should create their points from the manifold using one of those charts.)
</p>
TicketegourgoulhonFri, 20 Nov 2015 21:42:33 GMT
https://trac.sagemath.org/ticket/18529#comment:63
https://trac.sagemath.org/ticket/18529#comment:63
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:62" title="Comment 62">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I will then start my refactoring. I will try to make every change as granular as possible in the commits so we can cherrypick changes if you don't necessarily agree with them.
</p>
</blockquote>
<p>
OK very good.
Have you noticed that some doctests failed in the latest commit? This due to a typo: "coordintes" instead of "coordinates" in the replacement of
</p>
<pre class="wiki"> if self._restrictions != []:
substitutions = dict(zip(self._xx, coordinates))
</pre><p>
by
</p>
<pre class="wiki"> if self._restrictions:
substitutions = {x: coordintes[i] for i,x in enumerate(self._xx)}
</pre><p>
in <code>chart.py</code>. By the way, why is the second form better than the first one?
</p>
<blockquote class="citation">
<p>
How about this, let me make my changes and you can see what breaks or how much things slow down and we will use that to decide what we should do going forward. Does that sound reasonable to try?
</p>
</blockquote>
<p>
Yes, absolutely!
</p>
TickettscrimFri, 20 Nov 2015 23:17:05 GMT
https://trac.sagemath.org/ticket/18529#comment:64
https://trac.sagemath.org/ticket/18529#comment:64
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:63" title="Comment 63">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Have you noticed that some doctests failed in the latest commit? This due to a typo: "coordintes" instead of "coordinates" in the replacement of
</p>
<pre class="wiki"> if self._restrictions != []:
substitutions = dict(zip(self._xx, coordinates))
</pre><p>
by
</p>
<pre class="wiki"> if self._restrictions:
substitutions = {x: coordintes[i] for i,x in enumerate(self._xx)}
</pre><p>
in <code>chart.py</code>. By the way, why is the second form better than the first one?
</p>
</blockquote>
<p>
No, but I didn't run all of the doctests before I pushed (in part because I was planning to do a lot more work).
</p>
<p>
I think the latter is more clear in terms of code. I also thought I had run some timings before that the second was faster, but this is not the case:
</p>
<pre class="wiki">sage: k,v = range(1000), range(1000)
sage: %timeit d = dict(zip(k,v))
The slowest run took 5.50 times longer than the fastest. This could mean that an intermediate result is being cached
10000 loops, best of 3: 58.4 µs per loop
sage: %timeit d = {x: v[i] for i,x in enumerate(k)}
</pre><p>
So I will just revert this change (and any others like it).
</p>
TickettscrimWed, 25 Nov 2015 05:38:51 GMT
https://trac.sagemath.org/ticket/18529#comment:65
https://trac.sagemath.org/ticket/18529#comment:65
<p>
Status report: Still working on refactoring. I have had to go through a few iterations, but I think I've arrived to a model which will scale nicely with the additional manifold structures of smooth/differentiable/(almost)complex. I'm hoping to get it posted in a day or two.
</p>
<p>
Question, do you know what are the main bottleneck operations are? I'm wondering if there would be any benefits from doing a partial cythonization while I'm mucking around. If you don't know offhand, then I wouldn't worry about it.
</p>
TicketegourgoulhonWed, 25 Nov 2015 07:20:15 GMT
https://trac.sagemath.org/ticket/18529#comment:66
https://trac.sagemath.org/ticket/18529#comment:66
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:65" title="Comment 65">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Status report: Still working on refactoring. I have had to go through a few iterations, but I think I've arrived to a model which will scale nicely with the additional manifold structures of smooth/differentiable/(almost)complex. I'm hoping to get it posted in a day or two.
</p>
</blockquote>
<p>
OK very good. Thank you for working on this!
</p>
<blockquote class="citation">
<p>
Question, do you know what are the main bottleneck operations are? I'm wondering if there would be any benefits from doing a partial cythonization while I'm mucking around.
</p>
</blockquote>
<p>
In actual calculations, the main bottleneck is the simplification of symbolic expressions, which is performed in Lisp by Maxima. So I don't think that cythonizing would improve much here. On the other hand, parallelization over the components of vector fields (and more generally tensorial objects), as performed in <a class="closed ticket" href="https://trac.sagemath.org/ticket/18100" title="enhancement: Parallelization of computations on tensors on free modules (closed: fixed)">#18100</a>, helps a lot.
</p>
TickettscrimWed, 25 Nov 2015 21:11:37 GMTstatus, commit, branch changed
https://trac.sagemath.org/ticket/18529#comment:67
https://trac.sagemath.org/ticket/18529#comment:67
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
<li><strong>commit</strong>
changed from <em>0b08b114e03c063bd2e500ac900fd843fb12673b</em> to <em>0fb39df7fafe7f0a765bf73b3f34a6cb41e65c40</em>
</li>
<li><strong>branch</strong>
changed from <em>public/manifolds/top_manif_basics</em> to <em>u/tscrim/top_manifolds_refactor</em>
</li>
</ul>
<p>
Okay, I've refactored the code and it works up to "trivial" doctest failures. So almost all functionality on the outside hasn't changed, but internally, a lot of the logic within each function has changed at the cost of a slightly more complex class hierarchy (there might even be some more places to streamline things around too). Something else to do would be to define coercions between the respective subsets, but that we can do on a follow up since these are just sets at this point.
</p>
<p>
The biggest thing to note is that I have separated out the structure of the manifold into a separate class. This has several distinct advantages:
</p>
<ul><li>It has better encapsulation of data, which should result in fewer copies of the defining data needed and fewer duplication of functions.
</li><li>It could allow us to strength/weaken the structure on the manifold dynamically.
</li><li>We might only need to have one manifold class and one subset class, that way we don't have to duplicate documentation.
</li></ul><p>
The main drawback I see at the point is we don't expose the attributes of the structure (e.g., the differential order) directly from the manifold instance. However, if we want this behavior, then we can attach the appropriate data as (hidden) (class) attributes to keep some of the modularity; it just results in more classes.
</p>
<p>
I didn't want to change documentation until I knew you approved of this refactoring (or the alternative proposed above). Please tell me what you think and how you feel it fits with the differentiable manifolds part. I hope this does not cause too much trouble with rebasing.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=0fb39df7fafe7f0a765bf73b3f34a6cb41e65c40"><span class="icon"></span>0fb39df</a></td><td><code>Refactoring the code to separate out the structural part of the manifold.</code>
</td></tr></table>
TicketegourgoulhonSat, 28 Nov 2015 17:47:19 GMT
https://trac.sagemath.org/ticket/18529#comment:68
https://trac.sagemath.org/ticket/18529#comment:68
<p>
The refactoring with the new classes <code>AbstractObject</code>, <code>AbstractSet</code>, <code>ManifoldSubset</code> and <code>TopologicalSubmanifold</code> looks good. It clarify things, thanks! A suggestion regarding the naming: what about replacing <code>AbstractObject</code>, which sounds too general, by <code>AbstractNamedObject</code>, which would better reflect the class content?
</p>
<p>
Regarding the separation manifold/structure, I am wondering how this could fit with differentiable manifolds? Since the structure classes are singleton, they store things like the differential order but not properties specific to a given manifold, like the set of vector frames defined on the manifold or its module of vector fields. Then, how could one have a single class for all kind of manifolds? For instance, the class <code>DifferentiableManifold</code> introduced in <a class="closed ticket" href="https://trac.sagemath.org/ticket/18783" title="enhancement: Differentiable manifolds: basics (closed: fixed)">#18783</a> inherits from <code>TopologicalManifold</code> and has the additional attributes <code>_frames</code>, <code>_coframes</code>, <code>_frame_changes</code>, <code>_parallelizable_parts</code>, <code>_vector_field_modules</code>, etc., which have no meaning for a topological manifold. Besides, we don't want the user to write something like <code>v = M.structure().vector_frame(...)</code> instead of <code>v = M.vector_frame(...)</code>, do we?
</p>
TickettscrimSat, 28 Nov 2015 18:21:24 GMT
https://trac.sagemath.org/ticket/18529#comment:69
https://trac.sagemath.org/ticket/18529#comment:69
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:68" title="Comment 68">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
The refactoring with the new classes <code>AbstractObject</code>, <code>AbstractSet</code>, <code>ManifoldSubset</code> and <code>TopologicalSubmanifold</code> looks good. It clarify things, thanks! A suggestion regarding the naming: what about replacing <code>AbstractObject</code>, which sounds too general, by <code>AbstractNamedObject</code>, which would better reflect the class content?
</p>
</blockquote>
<p>
That is okay with me.
</p>
<blockquote class="citation">
<p>
Regarding the separation manifold/structure, I am wondering how this could fit with differentiable manifolds? Since the structure classes are singleton, they store things like the differential order but not properties specific to a given manifold, like the set of vector frames defined on the manifold or its module of vector fields.
</p>
</blockquote>
<p>
I wasn't imagining that they would be singletons, as they would carry instance information like differential order.
</p>
<blockquote class="citation">
<p>
Then, how could one have a single class for all kind of manifolds? For instance, the class <code>DifferentiableManifold</code> introduced in <a class="closed ticket" href="https://trac.sagemath.org/ticket/18783" title="enhancement: Differentiable manifolds: basics (closed: fixed)">#18783</a> inherits from <code>TopologicalManifold</code> and has the additional attributes <code>_frames</code>, <code>_coframes</code>, <code>_frame_changes</code>, <code>_parallelizable_parts</code>, <code>_vector_field_modules</code>, etc., which have no meaning for a topological manifold. Besides, we don't want the user to write something like <code>v = M.structure().vector_frame(...)</code> instead of <code>v = M.vector_frame(...)</code>, do we?
</p>
</blockquote>
<p>
Good point. Then rather these being instance objects, they should probably be mixin classes. This would help keep some separation of concerns, avoid some redundancies, and make it easy to manage the distinction between the full manifold and the submanifolds.
</p>
TicketegourgoulhonSun, 29 Nov 2015 10:33:33 GMT
https://trac.sagemath.org/ticket/18529#comment:70
https://trac.sagemath.org/ticket/18529#comment:70
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:69" title="Comment 69">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I wasn't imagining that they would be singletons,
</p>
</blockquote>
<p>
OK (I thought that since you were using singletons for the topological structure, you planed to use them for any kind of structure).
</p>
<blockquote class="citation">
<p>
as they would carry instance information like differential order.
</p>
</blockquote>
<p>
Regarding the specific case of the differential order, we may also consider that it is part of the structure per se and not of the instance, i.e. consider that a C<sup>2</sup>structure differs from a C<sup>4</sup>structure.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Then, how could one have a single class for all kind of manifolds? For instance, the class <code>DifferentiableManifold</code> introduced in <a class="closed ticket" href="https://trac.sagemath.org/ticket/18783" title="enhancement: Differentiable manifolds: basics (closed: fixed)">#18783</a> inherits from <code>TopologicalManifold</code> and has the additional attributes <code>_frames</code>, <code>_coframes</code>, <code>_frame_changes</code>, <code>_parallelizable_parts</code>, <code>_vector_field_modules</code>, etc., which have no meaning for a topological manifold. Besides, we don't want the user to write something like <code>v = M.structure().vector_frame(...)</code> instead of <code>v = M.vector_frame(...)</code>, do we?
</p>
</blockquote>
<p>
Good point. Then rather these being instance objects, they should probably be mixin classes. This would help keep some separation of concerns, avoid some redundancies, and make it easy to manage the distinction between the full manifold and the submanifolds.
</p>
</blockquote>
<p>
Could you please describe further how you would use mixin classes? and why this would be superior to the simple heritage <code>TopologicalManifold < DifferentiableManifold</code> ?
</p>
TicketegourgoulhonSun, 29 Nov 2015 10:34:14 GMTauthor changed
https://trac.sagemath.org/ticket/18529#comment:71
https://trac.sagemath.org/ticket/18529#comment:71
<ul>
<li><strong>author</strong>
changed from <em>Eric Gourgoulhon</em> to <em>Eric Gourgoulhon, Travis Scrimshaw</em>
</li>
</ul>
TickettscrimSun, 29 Nov 2015 16:03:44 GMT
https://trac.sagemath.org/ticket/18529#comment:72
https://trac.sagemath.org/ticket/18529#comment:72
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:70" title="Comment 70">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Could you please describe further how you would use mixin classes? and why this would be superior to the simple heritage <code>TopologicalManifold < DifferentiableManifold</code> ?
</p>
</blockquote>
<p>
If we just had a simple inheritance, then we'd have this as our hierarchy:
</p>
<pre class="wiki"> Abstract
/ \
Subset TopManifold
 ____/ 
 / 
TopSub DiffManifold
 ____/
 /
DiffSub
</pre><p>
However, with a mixin, we would have this:
</p>
<pre class="wiki"> Abstract
/ \
Subset TopManifold
 ____/ 
 / 
TopSub DiffMixin 
 / \ 
DiffSub DiffManifold
</pre><p>
In particular, notice that this does not introduce another diamond problem. It also makes it easier to add another class at the <code>Subset</code> and <code>TopManifold</code> level if we ever wanted to.
</p>
<p>
Ideally, I would like to abstract away the <code>Subset</code> parts to a mixin to completely avoid any diamonds, but I couldn't really figure out a good way to make that work.
</p>
TicketegourgoulhonSun, 29 Nov 2015 20:44:25 GMT
https://trac.sagemath.org/ticket/18529#comment:73
https://trac.sagemath.org/ticket/18529#comment:73
<p>
Thanks for these explanations.
</p>
TicketegourgoulhonSun, 29 Nov 2015 21:09:15 GMT
https://trac.sagemath.org/ticket/18529#comment:74
https://trac.sagemath.org/ticket/18529#comment:74
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:61" title="Comment 61">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
No, not at the moment: I've worked on the subsequent tickets to check if the introduced changes (in particular the removal of unique representation) propagate smoothly. So far, so good...
</p>
</blockquote>
<p>
Bad news: the removal of unique representation breaks parallel computations in <a class="closed ticket" href="https://trac.sagemath.org/ticket/19147" title="enhancement: Affine connections on smooth manifolds (closed: fixed)">#19147</a> (affine connections). For instance, in the branch of <a class="closed ticket" href="https://trac.sagemath.org/ticket/19147" title="enhancement: Affine connections on smooth manifolds (closed: fixed)">#19147</a>, if one performs
</p>
<pre class="wiki">sage: M = Manifold(3, 'M')
sage: X.<x,y,z> = M.chart()
sage: nab = M.affine_connection('nabla', r'\nabla')
sage: nab[0,0,1], nab[2,1,2] = x^2, y*z
sage: use_multiproc(2) # parallelization on 2 proc
sage: nab.riemann()
</pre><p>
one gets the error message:
</p>
<pre class="wiki">RuntimeError: There is a bug in the coercion code in Sage.
Both x (=Scalar field on the 3dimensional differentiable manifold M) and y (=Scalar field on the 3dimensional differentiable manifold M) are supposed to have identical parents but they don't.
In fact, x has parent 'Algebra of differentiable scalar fields on the 3dimensional differentiable manifold M'
whereas y has parent 'Algebra of differentiable scalar fields on the 3dimensional differentiable manifold M'
</pre><p>
and sage terminates badly (core dumped).
The reason is that the parallelization is using the pickling: the parallel iterator <code>sage.parallel.multiprocessing_sage.parallel_iter</code> invokes the function <code>sage.misc.fpickle.pickle_function</code>.
With unique representation, the pickling was fine and the above issue did not occur.
Note that parallelization of heavy computations like that of the Riemann tensor is really important!...
</p>
TickettscrimSun, 29 Nov 2015 22:57:32 GMT
https://trac.sagemath.org/ticket/18529#comment:75
https://trac.sagemath.org/ticket/18529#comment:75
<p>
I think what we will have to do is one (or more) of the following:
</p>
<ul><li>implement custom multiprocessing,
</li><li>move the parallelization code into the scalar fields algebra,
</li><li>implement a custom coercion scheme by overriding <code>__add__</code>, etc., or
</li><li>improve coercion support for non<code>UniqueRepresentation</code> objects.
</li></ul><p>
I think the first or second option will be the best possibility for this to work.
</p>
TicketegourgoulhonMon, 30 Nov 2015 10:03:01 GMTcc set
https://trac.sagemath.org/ticket/18529#comment:76
https://trac.sagemath.org/ticket/18529#comment:76
<ul>
<li><strong>cc</strong>
<em>mmancini</em> added
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:75" title="Comment 75">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I think what we will have to do is one (or more) of the following:
</p>
<ul><li>implement custom multiprocessing,
</li><li>move the parallelization code into the scalar fields algebra,
</li><li>implement a custom coercion scheme by overriding <code>__add__</code>, etc., or
</li><li>improve coercion support for non<code>UniqueRepresentation</code> objects.
</li></ul><p>
I think the first or second option will be the best possibility for this to work.
</p>
</blockquote>
<p>
The first looks to me much better than the second one: the nice feature of the parallelization framework as implemented in <a class="closed ticket" href="https://trac.sagemath.org/ticket/18100" title="enhancement: Parallelization of computations on tensors on free modules (closed: fixed)">#18100</a> is that it is implemented at the <code>Components</code> level, i.e. for any indexed set of ring elements, whatever the ring. It sounds bad to redefine this for the special case where the ring is an algebra of scalar fields. I shall discuss with Marco about implementing some multiprocessing that does not rely on pickling.
</p>
<p>
Another solution would be to revert to <code>UniqueRepresentation</code> for manifolds (once again!). Since now the user creates manifolds via the frontend function <code>Manifold</code> and not by a direct call to the manifold class constructor, the main issue raised at the end of <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:31" title="Comment 31">comment:31</a> could be overcome by the handling of the cache in <code>Manifold</code>. In this way
</p>
<pre class="wiki">sage: M1 = Manifold(2, 'M')
sage: M2 = Manifold(2, 'M')
</pre><p>
would result in two different objects, as desired, but
</p>
<pre class="wiki">sage: M1 == loads(dumps(M1))
</pre><p>
would still work, because the unpickling does not go through the function <code>Manifold</code>.
There remains the issue with <code>UniqueRepresentation</code> raised by Nils Bruin in the <a class="extlink" href="https://groups.google.com/d/msg/sagedevel/Vzfj1haZHho/sLfOJ3ujBQAJ"><span class="icon"></span>sagedevel thread</a>: <em>Avoid UniqueRepresentation if you can. It requires expensive processing of construction parameters and hence introduces bad overhead and it introduces "global variables" in a way that is much worse than global variables.</em>
However, in a typical work session, one should not create so many manifolds, so the "expensive processing of construction parameters" is not a too severe issue.
</p>
TickettscrimMon, 30 Nov 2015 16:59:29 GMT
https://trac.sagemath.org/ticket/18529#comment:77
https://trac.sagemath.org/ticket/18529#comment:77
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:76" title="Comment 76">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Another solution would be to revert to <code>UniqueRepresentation</code> for manifolds (once again!). Since now the user creates manifolds via the frontend function <code>Manifold</code> and not by a direct call to the manifold class constructor, the main issue raised at the end of <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:31" title="Comment 31">comment:31</a> could be overcome by the handling of the cache in <code>Manifold</code>. In this way
</p>
<pre class="wiki">sage: M1 = Manifold(2, 'M')
sage: M2 = Manifold(2, 'M')
</pre><p>
would result in two different objects, as desired, but
</p>
<pre class="wiki">sage: M1 == loads(dumps(M1))
</pre><p>
would still work, because the unpickling does not go through the function <code>Manifold</code>.
</p>
</blockquote>
<p>
This breaks pickling:
</p>
<pre class="wiki">sage: class Foo(UniqueRepresentation):
....: def __init__(self, i):
....: pass
....:
sage: F1 = Foo(1)
sage: Foo._clear_cache_()
sage: F2 = Foo(1)
sage: F1 is F2
False
sage: F1 == loads(dumps(F1))
False
sage: F2 == loads(dumps(F1))
True
</pre><p>
The reason is pickling still goes through the <code>UniqueRepresentation</code> cache. I am still convinced we should not be using <code>UniqueRepresentation</code> for the manifolds.
</p>
TicketegourgoulhonMon, 30 Nov 2015 17:16:40 GMT
https://trac.sagemath.org/ticket/18529#comment:78
https://trac.sagemath.org/ticket/18529#comment:78
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:77" title="Comment 77">tscrim</a>:
</p>
<blockquote class="citation">
<p>
This breaks pickling:
</p>
</blockquote>
<p>
Yes but by "handling of the cache" I had in mind not a call to <code>_clear_cache_</code> but rather adding a
unique tag to the constructor, such as the time tag that you already suggested. In this way, pickling would work. But I agree that this is a trick and that manifolds do not have a unique (mathematical) representation. However, given that in general we cannot decide whether two given manifolds are equal (isomorphic), making each of them unique with some tag could be an idea... What would be the drawbacks?
</p>
TicketegourgoulhonTue, 01 Dec 2015 13:01:17 GMT
https://trac.sagemath.org/ticket/18529#comment:79
https://trac.sagemath.org/ticket/18529#comment:79
<p>
Marco and I have discussed the thing this morning. It's clear that the <code>multiprocessing</code> Python module relies on pickling; some references are
</p>
<ul><li><a class="extlink" href="https://docs.python.org/2/library/multiprocessing.html"><span class="icon"></span>https://docs.python.org/2/library/multiprocessing.html</a>
</li><li><a class="extlink" href="http://matthewrocklin.com/blog/work/2013/12/05/ParallelismandSerialization/"><span class="icon"></span>http://matthewrocklin.com/blog/work/2013/12/05/ParallelismandSerialization/</a>
</li><li><a class="extlink" href="http://stackoverflow.com/questions/8804830/pythonmultiprocessingpicklingerror"><span class="icon"></span>http://stackoverflow.com/questions/8804830/pythonmultiprocessingpicklingerror</a>
</li></ul><p>
Therefore, unless one develops a brand new way of parallelizing Python codes, good pickling is required for parallelization. Given the importance of parallelization in tensor calculus, I am really considering reverting to <code>UniqueRepresentation</code> for the manifold classes. The argument goes as follows: from the <a class="extlink" href="https://groups.google.com/forum/#!msg/sagedevel/Vzfj1haZHho/"><span class="icon"></span>sagedevel discussion</a>, we get that manifolds must implement equality by identity (i.e. inherit from <code>WithEqualityById</code>), mostly because there exist no algorithm to decide whether two given manifolds are homeo/diffeomorphic (especially with manifolds with incomplete atlases, as we manipulate them). Then to have good pickling, i.e. to ensure <code>M == loads(dumps(M))</code>, the only way is to inherit from <code>CachedRepresentation</code> as well. Having both <code>WithEqualityById</code> and <code>CachedRepresentation</code> implies<code>UniqueRepresentation</code>.
</p>
TickettscrimTue, 01 Dec 2015 17:48:50 GMT
https://trac.sagemath.org/ticket/18529#comment:80
https://trac.sagemath.org/ticket/18529#comment:80
<p>
I did some exploring in the coercion code and some testing using this:
</p>
<div class="wikicode"><div class="code"><pre><span class="kn">from</span> <span class="nn">sage.structure.parent</span> <span class="kn">import</span> Parent
<span class="kn">from</span> <span class="nn">sage.structure.element_wrapper</span> <span class="kn">import</span> ElementWrapper
<span class="kn">from</span> <span class="nn">sage.misc.fast_methods</span> <span class="kn">import</span> WithEqualityById
<span class="kn">from</span> <span class="nn">sage.categories.additive_groups</span> <span class="kn">import</span> AdditiveGroups
<span class="k">class</span> <span class="nc">Foo</span><span class="p">(</span>Parent<span class="p">,</span> WithEqualityById<span class="p">):</span>
<span class="k">def</span> <span class="nf">__init__</span><span class="p">(</span><span class="bp">self</span><span class="p">):</span>
Parent<span class="o">.</span>__init__<span class="p">(</span><span class="bp">self</span><span class="p">,</span> category<span class="o">=</span>AdditiveGroups<span class="p">())</span>
<span class="k">def</span> <span class="nf">__reduce__</span><span class="p">(</span><span class="bp">self</span><span class="p">):</span>
<span class="k">return</span> <span class="p">(</span>Foo<span class="p">,</span> <span class="p">())</span>
<span class="k">def</span> <span class="nf">_coerce_map_from_</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> R<span class="p">):</span>
<span class="k">return</span> <span class="nb">isinstance</span><span class="p">(</span>R<span class="p">,</span> Foo<span class="p">)</span>
<span class="k">def</span> <span class="nf">_element_constructor_</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> x<span class="p">):</span>
<span class="k">if</span> <span class="nb">isinstance</span><span class="p">(</span>x<span class="p">,</span> ElementWrapper<span class="p">)</span> <span class="ow">and</span> <span class="nb">isinstance</span><span class="p">(</span>x<span class="o">.</span>parent<span class="p">(),</span> Foo<span class="p">):</span>
<span class="k">return</span> <span class="bp">self</span><span class="p">(</span>x<span class="o">.</span>value<span class="p">)</span>
<span class="k">return</span> <span class="nb">super</span><span class="p">(</span>Foo<span class="p">,</span> <span class="bp">self</span><span class="p">)</span><span class="o">.</span>_element_constructor_<span class="p">(</span>x<span class="p">)</span>
<span class="k">class</span> <span class="nc">Element</span><span class="p">(</span>ElementWrapper<span class="p">):</span>
<span class="k">def</span> <span class="nf">_add_</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> other<span class="p">):</span>
<span class="k">return</span> <span class="bp">self</span><span class="o">.</span>parent<span class="p">()(</span><span class="bp">self</span><span class="o">.</span>value<span class="o">+</span>other<span class="o">.</span>value<span class="p">)</span>
</pre></div></div><p>
which I think is a minimal example of the behavior you want. Pickling and coercion seem to work fine:
</p>
<pre class="wiki">sage: F = Foo()
sage: F2 = loads(dumps(F))
sage: F(1) + F2(1)
2
sage: F == F2
False
</pre><p>
Furthermore, from looking at your code, the reason why it now breaks is you're assuming unique representation by not passing the scalar field algebra as the first argument of the scalar field. So it's getting mismatches because the <code>domain.scalar_field_algebra()</code> may not be the scalar field algebra that was suppose to be creating that particular scalar field. So for the scalar fields, I would instead have the first argument be the corresponding algebra and get the domain from that (with appropriate handling of parameters for the methods which create the scalar fields). You might also consider caching the scalar field algebra that gets created for a particular manifold to have unique representation behavior.
</p>
TicketegourgoulhonTue, 01 Dec 2015 22:50:28 GMT
https://trac.sagemath.org/ticket/18529#comment:81
https://trac.sagemath.org/ticket/18529#comment:81
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:80" title="Comment 80">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I did some exploring in the coercion code and some testing using this:
</p>
</blockquote>
<p>
Thanks for the exploration.
The trick
</p>
<blockquote class="citation">
<div class="wikicode"><div class="code"><pre> <span class="k">def</span> <span class="nf">_coerce_map_from_</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> R<span class="p">):</span>
<span class="k">return</span> <span class="nb">isinstance</span><span class="p">(</span>R<span class="p">,</span> Foo<span class="p">)</span>
</pre></div></div></blockquote>
<p>
is too permissive to be applicable to scalar field algebras. For them, the coercion is currently based on the concept of restriction to a subdomain, i.e. there is a coerce map from C<sup>k</sup>(M) to C<sup>k</sup>(N) iff N is a subset of M (cf. the code of <code>_coerce_map_from_</code> in <code>scalar_field_algebra.py</code>). It is very desirable to keep this feature, which is mathematically neat.
</p>
<blockquote class="citation">
<p>
Furthermore, from looking at your code, the reason why it now breaks is you're assuming unique representation by not passing the scalar field algebra as the first argument of the scalar field. So it's getting mismatches because the <code>domain.scalar_field_algebra()</code> may not be the scalar field algebra that was suppose to be creating that particular scalar field. So for the scalar fields, I would instead have the first argument be the corresponding algebra and get the domain from that (with appropriate handling of parameters for the methods which create the scalar fields).
</p>
</blockquote>
<p>
I've done this (actually this should have been done before: it is cleaner to have the parent as first argument when constructing elements). But this does not solve the problem. There is a difference though: on the example of <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:74" title="Comment 74">comment:74</a>, we do no longer get the coercion error message but the ECL error message arising from Maxima; the latter was previously appearing after the coercion error message:
</p>
<pre class="wiki">;;;
;;; Stack overflow.
;;; Jumping to the outermost toplevel prompt
;;;
</pre><p>
and that a lot of text until <code>Abandon (core dumped)</code>. I've pushed the new code (i.e. with the algebra as first argument of scalar fields) to the branch of <a class="closed ticket" href="https://trac.sagemath.org/ticket/19147" title="enhancement: Affine connections on smooth manifolds (closed: fixed)">#19147</a>, in case you want to have a look.
</p>
<blockquote class="citation">
<p>
You might also consider caching the scalar field algebra that gets created for a particular manifold to have unique representation behavior.
</p>
</blockquote>
<p>
This was already the case (cf. the code of <code>DifferentiableManifold.scalar_field_algebra()</code>).
</p>
TickettscrimTue, 01 Dec 2015 23:29:15 GMT
https://trac.sagemath.org/ticket/18529#comment:82
https://trac.sagemath.org/ticket/18529#comment:82
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:81" title="Comment 81">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:80" title="Comment 80">tscrim</a>:
</p>
</blockquote>
<blockquote class="citation">
<p>
The trick
</p>
<blockquote class="citation">
<div class="wikicode"><div class="code"><pre> <span class="k">def</span> <span class="nf">_coerce_map_from_</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> R<span class="p">):</span>
<span class="k">return</span> <span class="nb">isinstance</span><span class="p">(</span>R<span class="p">,</span> Foo<span class="p">)</span>
</pre></div></div></blockquote>
<p>
is too permissive to be applicable to scalar field algebras. For them, the coercion is currently based on the concept of restriction to a subdomain, i.e. there is a coerce map from C<sup>k</sup>(M) to C<sup>k</sup>(N) iff N is a subset of M (cf. the code of <code>_coerce_map_from_</code> in <code>scalar_field_algebra.py</code>). It is very desirable to keep this feature, which is mathematically neat.
</p>
</blockquote>
<p>
I completely agree. I was just using it for illustrative purposes.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Furthermore, from looking at your code, the reason why it now breaks is you're assuming unique representation by not passing the scalar field algebra as the first argument of the scalar field. So it's getting mismatches because the <code>domain.scalar_field_algebra()</code> may not be the scalar field algebra that was suppose to be creating that particular scalar field. So for the scalar fields, I would instead have the first argument be the corresponding algebra and get the domain from that (with appropriate handling of parameters for the methods which create the scalar fields).
</p>
</blockquote>
<p>
I've done this (actually this should have been done before: it is cleaner to have the parent as first argument when constructing elements). But this does not solve the problem. There is a difference though: on the example of <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:74" title="Comment 74">comment:74</a>, we do no longer get the coercion error message but the ECL error message arising from Maxima; the latter was previously appearing after the coercion error message:
</p>
<pre class="wiki">;;;
;;; Stack overflow.
;;; Jumping to the outermost toplevel prompt
;;;
</pre><p>
and that a lot of text until <code>Abandon (core dumped)</code>. I've pushed the new code (i.e. with the algebra as first argument of scalar fields) to the branch of <a class="closed ticket" href="https://trac.sagemath.org/ticket/19147" title="enhancement: Affine connections on smooth manifolds (closed: fixed)">#19147</a>, in case you want to have a look.
</p>
</blockquote>
<p>
Hmm...that is very strange. Perhaps something with pickling and the interface?
</p>
<p>
However, I starting to lean towards reinstating the <code>UniqueRepresentation</code> behavior with initialization using a large random number via <code>manifold_constructor</code>. This will keep memory down with pickling because of all of the extra data that would need to be initialized after every pickling within the session. In an ideal world, I think we would implement our own variable system for the manifolds and hold some kind of global reference to that. I will think about this on my way home right now.
</p>
<p>
Question, should override the <code>__copy__</code> for the manifold (at least, I feel there might be some use cases for building two manifolds with a common base atlas)?
</p>
TickettscrimWed, 02 Dec 2015 03:05:02 GMT
https://trac.sagemath.org/ticket/18529#comment:83
https://trac.sagemath.org/ticket/18529#comment:83
<p>
Okay, let's revert back to having <code>UniqueRepresentation</code> because it will work as a session/variable manager. I don't think we should spend a lot of time doing that as we can hack around it (even though it is in effect what we are doing/should do). I don't like it because it is a hack, but it seems like the best option for now. Unless you want to directly use Python's multiprocessing with shared memory for the corresponding parents to avoid pickling, which I would still somewhat prefer in a way so we don't have to do this hack. (Multithreading in Python has shared memory, but unfortunately it can't run on multiple processors because of the GIL.) Anyways, I leave the decision up to you.
</p>
TicketegourgoulhonWed, 02 Dec 2015 10:34:01 GMT
https://trac.sagemath.org/ticket/18529#comment:84
https://trac.sagemath.org/ticket/18529#comment:84
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:82" title="Comment 82">tscrim</a>:
</p>
<blockquote class="citation">
<p>
However, I starting to lean towards reinstating the <code>UniqueRepresentation</code> behavior with initialization using a large random number via <code>manifold_constructor</code>. This will keep memory down with pickling because of all of the extra data that would need to be initialized after every pickling within the session.
</p>
</blockquote>
<p>
OK.
</p>
<blockquote class="citation">
<p>
In an ideal world, I think we would implement our own variable system for the manifolds and hold some kind of global reference to that.
</p>
</blockquote>
<p>
Yes this could be something to implement in the future.
</p>
<blockquote class="citation">
<p>
Question, should override the <code>__copy__</code> for the manifold (at least, I feel there might be some use cases for building two manifolds with a common base atlas)?
</p>
</blockquote>
<p>
Yes, a use case could be trying different things by extending the atlas of a given manifold. The initial manifold, arising for instance from some manifold catalog, could be copied to serve as a reference for the various trials.
</p>
TicketegourgoulhonWed, 02 Dec 2015 10:50:20 GMT
https://trac.sagemath.org/ticket/18529#comment:85
https://trac.sagemath.org/ticket/18529#comment:85
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:83" title="Comment 83">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Okay, let's revert back to having <code>UniqueRepresentation</code> because it will work as a session/variable manager. I don't think we should spend a lot of time doing that as we can hack around it (even though it is in effect what we are doing/should do). I don't like it because it is a hack, but it seems like the best option for now. Unless you want to directly use Python's multiprocessing with shared memory for the corresponding parents to avoid pickling, which I would still somewhat prefer in a way so we don't have to do this hack. (Multithreading in Python has shared memory, but unfortunately it can't run on multiple processors because of the GIL.) Anyways, I leave the decision up to you.
</p>
</blockquote>
<p>
Yes, I think it is reasonable to revert to <code>UniqueRepresentation</code> at this stage, leaving Python's multiprocessing without pickling for a future development, exploring meanwhile other ways of parallelization (IPython parallel framework ?). I've rerun this morning some benchmarks on the commit of <a class="closed ticket" href="https://trac.sagemath.org/ticket/19209" title="enhancement: PseudoRiemannian metrics on smooth manifolds (closed: fixed)">#19209</a> based on <code>UniqueRepresentation</code> (commit <a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=82f6f495bb6f729624dd8396208f5a3c67e2fa8d"><span class="icon"></span>82f6f495b</a>) merged into Sage 6.10.beta6: the gain in the computation of the Riemann tensor of a nontrivial metric (4dimensional <a class="extlink" href="http://sagemanifolds.obspm.fr/examples/html/SM_Kerr.html"><span class="icon"></span>Kerr metric</a>) is really significant: a factor of 4 when using 8 cores instead of 1 (30 s instead of 115 s on Xeon E52623 CPUs).
</p>
TicketegourgoulhonMon, 14 Dec 2015 14:41:11 GMTcommit, branch, milestone changed
https://trac.sagemath.org/ticket/18529#comment:86
https://trac.sagemath.org/ticket/18529#comment:86
<ul>
<li><strong>commit</strong>
changed from <em>0fb39df7fafe7f0a765bf73b3f34a6cb41e65c40</em> to <em>c5f35afa41b2dac89471ba63d80fd2ae8eebbc2f</em>
</li>
<li><strong>branch</strong>
changed from <em>u/tscrim/top_manifolds_refactor</em> to <em>public/manifolds/top_manif_basics</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage6.10</em> to <em>sage6.11</em>
</li>
</ul>
<p>
I've started to work again on the ticket (not finished yet) and have one question: given the inheritance diagram of <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:72" title="Comment 72">comment:72</a>, shall we rename the class <code>Manifold</code> to <code>TopologicalManifold</code> ? Then, can we revert to <code>Manifold</code> for <code>manifold_constructor</code> ?
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=d3e5d4d31b0048ee51674ff28d4f5e26e85d2a8f"><span class="icon"></span>d3e5d4d</a></td><td><code>Revert to UniqueRepresentation for topological manifolds</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=85d03dc251d1c30871010a2b0729234abf8253a0"><span class="icon"></span>85d03dc</a></td><td><code>Change the argument type to structure in Manifold</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=5251ef034afde1b6feea50ab0846915a5e771f96"><span class="icon"></span>5251ef0</a></td><td><code>Remove method _test_pickling from class TopologicalManifoldPoint</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=2ca24ff521203f8cd833cca455610dd17ddd02c9"><span class="icon"></span>2ca24ff</a></td><td><code>Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into Sage 6.10.rc1</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=cdfa817ba2779f89cd720d05b8738edd66d422d6"><span class="icon"></span>cdfa817</a></td><td><code>Merge branch u/tscrim/top_manifolds_refactor into Sage 6.10.rc1 + public/manifolds/top_manif_basics</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=c5f35afa41b2dac89471ba63d80fd2ae8eebbc2f"><span class="icon"></span>c5f35af</a></td><td><code>Make AbstractSet inherit from UniqueRepresentation; correct doctests; start to change documentation.</code>
</td></tr></table>
TickettscrimMon, 14 Dec 2015 14:57:32 GMT
https://trac.sagemath.org/ticket/18529#comment:87
https://trac.sagemath.org/ticket/18529#comment:87
<p>
IMO, it is more clear to have in the code <code>manifold_constructor</code> as that is suppose to be a toplevel function (which we import at <code>Manifold</code> into the global namespace). Yet I think that is not a strong argument as it introduces a disassociation. Given that we will be using mixin classes, we should revert <code>Manifold</code> back to <code>TopologicalManifold</code>. So I leave the other name change up to you.
</p>
TicketgitTue, 15 Dec 2015 16:11:46 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:88
https://trac.sagemath.org/ticket/18529#comment:88
<ul>
<li><strong>commit</strong>
changed from <em>c5f35afa41b2dac89471ba63d80fd2ae8eebbc2f</em> to <em>3a525002469e0ef676a0dcf48f8f7f8683b85853</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=3a525002469e0ef676a0dcf48f8f7f8683b85853"><span class="icon"></span>3a52500</a></td><td><code>Some class renaming; add more examples and doctests (full coverage)</code>
</td></tr></table>
TicketegourgoulhonTue, 15 Dec 2015 16:44:25 GMT
https://trac.sagemath.org/ticket/18529#comment:89
https://trac.sagemath.org/ticket/18529#comment:89
<p>
In the above commit, I've
</p>
<ul><li>renamed class <code>Manifold</code> to <code>TopologicalManifold</code>
</li><li>renamed function <code>manifold_constructor</code> to <code>Manifold</code>, i.e. to the name by which it is imported
to the global namespace (IMHO, this makes the documentation more clear for a beginner:
previously, <code>sage: Manifold??</code> was returning something like <code>def manifold_constructor</code>...
Also the html doc looks better with a single name)
</li><li>renamed class <code>TopologicalSubmanifold</code> to <code>OpenTopologicalSubmanifold</code>, to avoid any ambiguity with
embedded submanifolds of lower dimension
</li><li>revert the attribute <code>_open_covers</code> to a list of lists (instead of a set of frozensets), mostly to
ensure the reproductibility of computations from one machine to another one, i.e. this garantees
that <code>for ... in</code> loops are always performed in the same way (maybe this is something to
be discussed further...). Moreover, the mathematical definition of an
open cover refers to an indexed family, which corresponds more to a list than to a frozenset.
</li><li>removed the method <code>is_open()</code> from <code>AbstractSet</code> (where it was always returning <code>True</code>!) and
implemented it in <code>TopologicalManifold</code>. Now it is on the same footing as methods <code>superset</code>,
<code>union</code> and <code>intersection</code>, which are not implemented in the base class <code>AbstractSet</code> but in each
of the subclasses <code>TopologicalManifold</code> and <code>ManifoldSubset</code>
</li><li>removed the (unused) argument <code>category</code> from <code>ManifoldSubset.__init__</code>
</li><li>added an example to illustrate the class <code>OpenTopologicalSubmanifold</code>, as well as enough doctests
to get a full coverage
</li></ul><p>
To answer to some of your questions in the code:
</p>
<ul><li>I don't think that <code>ManifoldPoint</code> must inherit from <code>AbstractNamedObject</code> since the name is
not essential in the definition of a point (it can have no name), contrary to that of a subset.
Moreover, it would slower the creation of points, which can be a problem (no such problem for
parent objects, which are not expected to be massively created)
</li><li>Yes, I think we need <code>list_of_subsets</code> to have something duplicable from one machine to another one
(cf. remark above about <code>_open_covers</code>).
</li></ul><p>
The test suite of <code>ManifoldSubset</code> and <code>OpenTopologicalSubmanifold</code> fails because of the lack of a method <code>lift</code>. What this method shall be? I guess we have to wait for the morphism ticket (<a class="closed ticket" href="https://trac.sagemath.org/ticket/18725" title="enhancement: Topological manifolds: morphisms (closed: fixed)">#18725</a>) to implement it.
</p>
TicketegourgoulhonWed, 16 Dec 2015 10:03:02 GMTdescription changed
https://trac.sagemath.org/ticket/18529#comment:90
https://trac.sagemath.org/ticket/18529#comment:90
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=90">diff</a>)
</li>
</ul>
TicketgitWed, 16 Dec 2015 16:27:48 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:91
https://trac.sagemath.org/ticket/18529#comment:91
<ul>
<li><strong>commit</strong>
changed from <em>3a525002469e0ef676a0dcf48f8f7f8683b85853</em> to <em>cb534171ae64a7f61a4ea53f41982de0e9eecbd2</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=cb534171ae64a7f61a4ea53f41982de0e9eecbd2"><span class="icon"></span>cb53417</a></td><td><code>Add argument full_name to AbstractNamedObject; remove _repr_() from all parent classes; improve documentation</code>
</td></tr></table>
TicketegourgoulhonWed, 16 Dec 2015 16:35:48 GMT
https://trac.sagemath.org/ticket/18529#comment:92
https://trac.sagemath.org/ticket/18529#comment:92
<p>
In the above commit, the class <code>AbstractNamedObject</code> has been improved so that there is no need to redefine its method <code>_repr_()</code> in the classes that inherit from it. Consequently, methods <code>_repr_()</code> have been removed from all the set and manifold classes. For all these classes, <code>_repr_()</code> and <code>_latex_()</code> are now on the same footing: they use the <code>AbstractNamedObject</code> implementation. This will also be used for the scalar field algebras of <a class="closed ticket" href="https://trac.sagemath.org/ticket/18640" title="enhancement: Topological manifolds: scalar fields (closed: fixed)">#18640</a>.
</p>
TicketgitFri, 18 Dec 2015 12:50:52 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:93
https://trac.sagemath.org/ticket/18529#comment:93
<ul>
<li><strong>commit</strong>
changed from <em>cb534171ae64a7f61a4ea53f41982de0e9eecbd2</em> to <em>c38ae80cbd8032cf7041259284b6f646265d1e42</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=c38ae80cbd8032cf7041259284b6f646265d1e42"><span class="icon"></span>c38ae80</a></td><td><code>Suppress the (unused) argument category in OpenTopologicalSubmanifold; minor doc improvements</code>
</td></tr></table>
TicketegourgoulhonFri, 18 Dec 2015 12:58:03 GMT
https://trac.sagemath.org/ticket/18529#comment:94
https://trac.sagemath.org/ticket/18529#comment:94
<p>
<em>Status report:</em> I've propagated the refactoring introduced in this ticket up to <a class="closed ticket" href="https://trac.sagemath.org/ticket/18843" title="enhancement: Differentiable manifolds: vector fields and tensor fields (closed: fixed)">#18843</a> (tensor fields on diff. manifolds). In particular, I've introduced the mixin class <code>DifferentiableMixin</code> in <a class="closed ticket" href="https://trac.sagemath.org/ticket/18783" title="enhancement: Differentiable manifolds: basics (closed: fixed)">#18783</a> to deal with differentiable manifolds and open submanifolds, in agreement with the diagram of <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:72" title="Comment 72">comment:72</a>. So far, so good...
</p>
TickettscrimFri, 18 Dec 2015 15:36:57 GMT
https://trac.sagemath.org/ticket/18529#comment:95
https://trac.sagemath.org/ticket/18529#comment:95
<p>
Sorry for the delay in getting to this.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:89" title="Comment 89">egourgoulhon</a>:
</p>
<blockquote class="citation">
<ul><li>revert the attribute <code>_open_covers</code> to a list of lists (instead of a set of frozensets), mostly to
ensure the reproductibility of computations from one machine to another one, i.e. this garantees
that <code>for ... in</code> loops are always performed in the same way (maybe this is something to
be discussed further...). Moreover, the mathematical definition of an
open cover refers to an indexed family, which corresponds more to a list than to a frozenset.
</li></ul></blockquote>
<p>
The <code>set</code> of <code>frozenset</code>s will have a faster lookup as manifolds get larger. I think this is still valid mathematically because the indexed family does not have to be ordered as far as I know. So it would be more like a <code>dict</code>, but I don't think we care about the indexing; if we did, then we should go to a <code>list</code>. For the doctests, we can just do a <code>sorted</code> or we have an internal method which returns it in a canonical way (i.e., ordered via its string representation).
</p>
<blockquote class="citation">
<ul><li>removed the method <code>is_open()</code> from <code>AbstractSet</code> (where it was always returning <code>True</code>!) and
implemented it in <code>TopologicalManifold</code>.
</li></ul></blockquote>
<p>
Perhaps the default value for <code>is_open</code> in <code>AbstractSet</code> should return an <code>Unknown</code>, be an <code>@abstract_method</code>, or raise a <code>NotImplementedError</code>? I'm in favor of the first one.
</p>
<blockquote class="citation">
<ul><li>removed the (unused) argument <code>category</code> from <code>ManifoldSubset.__init__</code>
</li></ul></blockquote>
<p>
This was previously there as I was wanting to call the <code>__init__</code>, but since I copied the constructor, this has become unnecessary. Although now I'm thinking we should have an <code>_init_subset</code> method to remove the duplicated code and have that be called by both <code>ManifoldSubset.__init__</code> and <code>OpenTopologicalSubmanifold.__init__</code>.
</p>
<blockquote class="citation">
<ul><li>added an example to illustrate the class <code>OpenTopologicalSubmanifold</code>, as well as enough doctests
to get a full coverage
</li></ul></blockquote>
<p>
Thank you.
</p>
<blockquote class="citation">
<p>
To answer to some of your questions in the code:
</p>
<ul><li>I don't think that <code>ManifoldPoint</code> must inherit from <code>AbstractNamedObject</code> since the name is
not essential in the definition of a point (it can have no name), contrary to that of a subset.
Moreover, it would slower the creation of points, which can be a problem (no such problem for
parent objects, which are not expected to be massively created)
</li></ul></blockquote>
<p>
Fair enough. Although if that extra little function call is going to matter, then I think we should cythonize <code>ManifoldPoint</code>.
</p>
<blockquote class="citation">
<ul><li>Yes, I think we need <code>list_of_subsets</code> to have something duplicable from one machine to another one
(cf. remark above about <code>_open_covers</code>).
</li></ul></blockquote>
<p>
Same comments about <code>_open_covers</code> as well.
</p>
<blockquote class="citation">
<p>
The test suite of <code>ManifoldSubset</code> and <code>OpenTopologicalSubmanifold</code> fails because of the lack of a method <code>lift</code>. What this method shall be? I guess we have to wait for the morphism ticket (<a class="closed ticket" href="https://trac.sagemath.org/ticket/18725" title="enhancement: Topological manifolds: morphisms (closed: fixed)">#18725</a>) to implement it.
</p>
</blockquote>
<p>
<code>lift</code> can just be a method that creates the corresponding point in the ambient manifold, and similarly for <code>retract</code>. Once we have <a class="closed ticket" href="https://trac.sagemath.org/ticket/18725" title="enhancement: Topological manifolds: morphisms (closed: fixed)">#18725</a>, we can convert it to a <code>@lazy_attribute</code> which is set to the morphism (note that this does not change the API).
</p>
<p>
I don't reall agree with <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:92" title="Comment 92">comment:92</a>. I feel that this only removes a few docstrings, adds some internal complexity, and increases memory usage (although since we aren't really using this for <code>TopologicalPoint</code>, the memory usage is less of a concern). In principle, it is a mixin class, but it just acts like an ABC. So it is perfectly valid to override its <code>_repr_</code>.
</p>
<p>
I'm glad to hear the mixin class idea is working.
</p>
TicketegourgoulhonFri, 18 Dec 2015 17:26:09 GMT
https://trac.sagemath.org/ticket/18529#comment:96
https://trac.sagemath.org/ticket/18529#comment:96
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:95" title="Comment 95">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Sorry for the delay in getting to this.
</p>
</blockquote>
<p>
No problem.
</p>
<blockquote class="citation">
<p>
The <code>set</code> of <code>frozenset</code>s will have a faster lookup as manifolds get larger. I think this is still valid mathematically because the indexed family does not have to be ordered as far as I know. So it would be more like a <code>dict</code>, but I don't think we care about the indexing; if we did, then we should go to a <code>list</code>. For the doctests, we can just do a <code>sorted</code> or we have an internal method which returns it in a canonical way (i.e., ordered via its string representation).
</p>
</blockquote>
<p>
It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on nonparallelizable manifolds makes use of open covers.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
The test suite of <code>ManifoldSubset</code> and <code>OpenTopologicalSubmanifold</code> fails because of the lack of a method <code>lift</code>. What this method shall be? I guess we have to wait for the morphism ticket (<a class="closed ticket" href="https://trac.sagemath.org/ticket/18725" title="enhancement: Topological manifolds: morphisms (closed: fixed)">#18725</a>) to implement it.
</p>
</blockquote>
<p>
<code>lift</code> can just be a method that creates the corresponding point in the ambient manifold, and similarly for <code>retract</code>. Once we have <a class="closed ticket" href="https://trac.sagemath.org/ticket/18725" title="enhancement: Topological manifolds: morphisms (closed: fixed)">#18725</a>, we can convert it to a <code>@lazy_attribute</code> which is set to the morphism (note that this does not change the API).
</p>
</blockquote>
<p>
OK, thanks.
</p>
<blockquote class="citation">
<p>
I don't reall agree with <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:92" title="Comment 92">comment:92</a>. I feel that this only removes a few docstrings, adds some internal complexity, and increases memory usage (although since we aren't really using this for <code>TopologicalPoint</code>, the memory usage is less of a concern). In principle, it is a mixin class, but it just acts like an ABC. So it is perfectly valid to override its <code>_repr_</code>.
</p>
</blockquote>
<p>
OK, I will revert to the previous version.
</p>
TickettscrimFri, 18 Dec 2015 17:33:06 GMT
https://trac.sagemath.org/ticket/18529#comment:97
https://trac.sagemath.org/ticket/18529#comment:97
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:96" title="Comment 96">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:95" title="Comment 95">tscrim</a>:
</p>
<blockquote class="citation">
<p>
The <code>set</code> of <code>frozenset</code>s will have a faster lookup as manifolds get larger. I think this is still valid mathematically because the indexed family does not have to be ordered as far as I know. So it would be more like a <code>dict</code>, but I don't think we care about the indexing; if we did, then we should go to a <code>list</code>. For the doctests, we can just do a <code>sorted</code> or we have an internal method which returns it in a canonical way (i.e., ordered via its string representation).
</p>
</blockquote>
<p>
It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on nonparallelizable manifolds makes use of open covers.
</p>
</blockquote>
<p>
But does it depend on the ordering of subsets that give the open cover, or which open cover it takes? Does the API for that have any control over what order it was given (rather than the order in which the open covers were created)? What I'm getting at is that it shouldn't depend upon these things, right? Or are you trying to put a total ordering on these vector fields, and if so, why?
</p>
TicketegourgoulhonMon, 21 Dec 2015 10:17:12 GMT
https://trac.sagemath.org/ticket/18529#comment:98
https://trac.sagemath.org/ticket/18529#comment:98
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:97" title="Comment 97">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:96" title="Comment 96">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on nonparallelizable manifolds makes use of open covers.
</p>
</blockquote>
<p>
But does it depend on the ordering of subsets that give the open cover, or which open cover it takes?
</p>
</blockquote>
<p>
For this specific case (comparison), it depends on the open cover, not on the ordering of the subsets in it.
</p>
<blockquote class="citation">
<p>
Does the API for that have any control over what order it was given (rather than the order in which the open covers were created)?
</p>
</blockquote>
<p>
No there is no control at the API level, because the API is <code>__eq__</code> (one cannot pass extra parameters)
</p>
<blockquote class="citation">
<p>
What I'm getting at is that it shouldn't depend upon these things, right?
</p>
</blockquote>
<p>
Yes, in the ideal mathematical world. In the much less ideal symbolic world, this is not so true... The argument goes as follows: in order to have a pretty fast <code>__eq__</code> operator for tensor fields, the comparison is not performed on all the subdomains where the tensor fields have known (or computable) restrictions, but only on a single open cover, which is mathematically sufficient. So one has to pick an open cover. By making <code>_open_covers</code> a list, we ensure that the picked one is always the same. The key point is that sometimes the comparison fails because of simplification issues (no normal form for symbolic expressions), i.e. <code>__eq__</code> returns <code>False</code> while the two tensor fields are mathematically equals. This is not satisfactory but I guess this is inescapable when dealing with symbolics. At least, by making <code>_open_covers</code> a list, we garantee that the output of <code>__eq__</code>, whatever its mathematical relevance, is consistent from one machine to the next one.
</p>
TickettscrimMon, 21 Dec 2015 20:10:54 GMT
https://trac.sagemath.org/ticket/18529#comment:99
https://trac.sagemath.org/ticket/18529#comment:99
<p>
Then how about we have a <code>_distinguished_open_cover</code>, which is set to the first open cover that gets added? IMO, this will be
</p>
<ul><li>lightweight (just a pointer);
</li><li>(marginally) faster (only an attribute lookup instead of attribute plus index grab; granted, this speedup will probably never be noticable);
</li><li>keep the <code>O(1)</code> checking speed of something being an open cover;
</li><li>and more robust when doing the comparisons (we check to see if the distinguished open cover is an open cover of the other one, so we don't have to worry about things breaking if something ends up being out of order in the creating of the open cover list).
</li></ul><p>
It means one additional attribute we'll have to manage, but I think that is an epsilon cost.
</p>
TicketegourgoulhonWed, 23 Dec 2015 16:24:10 GMT
https://trac.sagemath.org/ticket/18529#comment:100
https://trac.sagemath.org/ticket/18529#comment:100
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:99" title="Comment 99">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Then how about we have a <code>_distinguished_open_cover</code>, which is set to the first open cover that gets added?
</p>
</blockquote>
<p>
Well the tensor field <code>__eq__</code> mentionned above is only one of the possible examples where open covers could be involved. One may imagine that they are also used in tensor field <code>_add_</code> for instance. This is not the case at the moment, but this is certainly something to be considered when discussing the tensor field arithmetics.
So, at this stage, I would prefer a sequence (i.e. a Python list) of open covers instead of a set with a distinguished element. IMO, this offers a greater flexibility for future operations, like <code>_add_</code>. With a sequence, you may always say that your distinguished element is the first one, and if, for some reason, the attempted operation fails with it, you may try with the second one, and so on (while if you have only a distinguished element, you don't know what to do in case of failure).
So could we stay with a list at this stage?
</p>
TickettscrimWed, 23 Dec 2015 17:33:22 GMT
https://trac.sagemath.org/ticket/18529#comment:101
https://trac.sagemath.org/ticket/18529#comment:101
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:100" title="Comment 100">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
So could we stay with a list at this stage?
</p>
</blockquote>
<p>
Since you feel strongly about it, then we can leave it be. I got reminded recently about something with premature optimization and the root of all evil... :P
</p>
TicketgitSun, 03 Jan 2016 19:49:14 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:102
https://trac.sagemath.org/ticket/18529#comment:102
<ul>
<li><strong>commit</strong>
changed from <em>c38ae80cbd8032cf7041259284b6f646265d1e42</em> to <em>19caedb4d055887ad33ab2ab7b051d166bd58c90</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=069baf4c9bddffee6a31b58884dc383f27869248"><span class="icon"></span>069baf4</a></td><td><code>Merge branch 'public/manifolds/top_manif_basics' into Sage 7.0.beta2.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=19caedb4d055887ad33ab2ab7b051d166bd58c90"><span class="icon"></span>19caedb</a></td><td><code>Revert to AbstractNamedObject without argument full_name; restore _repr_() in manifold classes</code>
</td></tr></table>
TicketegourgoulhonSun, 03 Jan 2016 19:54:24 GMT
https://trac.sagemath.org/ticket/18529#comment:103
https://trac.sagemath.org/ticket/18529#comment:103
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:95" title="Comment 95">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I don't reall agree with <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:92" title="Comment 92">comment:92</a>. I feel that this only removes a few docstrings, adds some internal complexity, and increases memory usage (although since we aren't really using this for <code>TopologicalPoint</code>, the memory usage is less of a concern). In principle, it is a mixin class, but it just acts like an ABC. So it is perfectly valid to override its <code>_repr_</code>.
</p>
</blockquote>
<p>
In the above commit, I've reverted to the previous version of <code>AbstractNamedObject</code> and have restored the <code>_repr_</code> methods.
</p>
TickettscrimSun, 03 Jan 2016 23:40:14 GMT
https://trac.sagemath.org/ticket/18529#comment:104
https://trac.sagemath.org/ticket/18529#comment:104
<p>
Okay,thanks. So the only thing left to do is implement
</p>
<div class="wikicode"><div class="code"><pre><span class="k">def</span> <span class="nf">lift</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> p<span class="p">):</span>
<span class="k">return</span> <span class="bp">self</span><span class="o">.</span>_manifold<span class="p">(</span>p<span class="p">)</span>
<span class="k">def</span> <span class="nf">retract</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> p<span class="p">):</span>
<span class="k">return</span> <span class="bp">self</span><span class="p">(</span>p<span class="p">)</span>
</pre></div></div><p>
for the subset class (not that we will really use them). Once that is done, I believe the only thing left is for me to take one last look through everything. Thank you for all your work on this.
</p>
TicketgitMon, 04 Jan 2016 11:14:22 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:105
https://trac.sagemath.org/ticket/18529#comment:105
<ul>
<li><strong>commit</strong>
changed from <em>19caedb4d055887ad33ab2ab7b051d166bd58c90</em> to <em>3cd03a48d847e12745ed8c25b23f19db141c179a</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=3cd03a48d847e12745ed8c25b23f19db141c179a"><span class="icon"></span>3cd03a4</a></td><td><code>Add methods lift() and retract() to ManifoldSubset; add method __eq__() in CoordChange</code>
</td></tr></table>
TicketegourgoulhonMon, 04 Jan 2016 11:17:23 GMT
https://trac.sagemath.org/ticket/18529#comment:106
https://trac.sagemath.org/ticket/18529#comment:106
<p>
The above commit implements the methods <code>lift</code> and <code>retract</code> as you suggested; it also implements the method <code>__eq__</code> in class <code>CoordChange</code>, which fixes the <code>_test_pickling</code> of that class.
</p>
TicketegourgoulhonMon, 04 Jan 2016 20:53:48 GMT
https://trac.sagemath.org/ticket/18529#comment:107
https://trac.sagemath.org/ticket/18529#comment:107
<p>
I've continued to propagate the refactorization initialized in <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:67" title="Comment 67">comment:67</a> up to <a class="closed ticket" href="https://trac.sagemath.org/ticket/19124" title="enhancement: Curves in differentiable manifolds (closed: fixed)">#19124</a> (curves in differentiable manifolds) in the ticket hierarchy of <a class="new ticket" href="https://trac.sagemath.org/ticket/18528" title="task: SageManifolds metaticket (new)">#18528</a>. At this stage, the class hierarchy is an extension of the second diagram of <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:72" title="Comment 72">comment:72</a>:
</p>
<pre class="wiki">Hierarchy1:
AbstractSet
/ \
ManifoldSubset TopologicalManifold
 ____/ 
 / 
OpenTopSubmanifold 
 
 DifferentiableMixin 
 / \ 
OpenDiffSubmanifold DifferentiableManifold
 
 IntervalMixin 
 / \ 
OpenSubinterval OpenInterval

RealLine
</pre><p>
Now, prior to the refactoring, the class hierarchy was
</p>
<pre class="wiki">Hierarchy2:
ManifoldSubset

TopologicalManifold

DifferentiableManifold

OpenInterval

RealLine
</pre><p>
It is clear that Hierarchy2 is much simpler than Hierarchy1: it involves much less classes and has no multiple heritage issues. Moreover, Hierarchy2 reflects fully the mathematics: the real line <strong>R</strong> is the open interval (oo, +oo), which is a 1dimensional differentiable manifold, which is a 1dimensional topological manifold, which is a subset of a topological manifold (itself). Within Hierarchy2, open subsets are not handled by a specific class, but directly by the manifold classes: an open subset of a topological (resp. differentiable) manifold is created as an instance of <code>TopologicalManifold</code> (resp. <code>DifferentiableManifold</code>). Again, this reflects the mathematics since an open subset of a manifold inherits the manifold structure.
The classes of Hierarchy2 have an attribute <code>_manifold</code> (which could also be called <code>_ambient</code>), which is the only piece that permits to distinguish between a manifold per se (<code>_manifold</code> is then set to <code>self</code>) and an open subset of a larger manifold (<code>_manifold</code> is then set to the latter). In Hierarchy1, the attribute <code>_manifold</code> exists only for the subset classes. For motivating Hierarchy1, you said that it avoids tests of the type <code>self is self._manifold</code>. Now, in Hierarchy2, these tests appear only in two places: (i) the <code>_repr_</code> method (to return "Open subset of..." instead of "Manifold...") and (ii) the <code>union</code> and <code>intersection</code> methods. Hence the question: are there any other reason to introduce Hierarchy1? Given its complexity, as compared with Hierarchy2, I am wondering whether it is worth continuing with it...
Note that with Hierarchy2, one can still distinguish open subsets from the ambient manifold by the category: <code>Manifolds(K).Subobjects()</code> for open subsets versus <code>Manifolds(K)</code> for the ambient manifold. There is no need to have a separate class for this.
Another argument in favor of Hierarchy2 regards the construction of a new manifold by the disjoint union of two manifolds (not implemented yet): suppose we have two manifolds <em>A</em> and <em>B</em> of the same dimension over the same topological field <strong>K</strong>. We can then form the manifold <em>M = A</em> U <em>B</em> and consider <em>A</em> and <em>B</em> as open subsets of <em>M</em> by simplify changing their attribute <code>_manifold</code> from <code>self</code> to <em>M</em>. With Hierarchy1, one would need to create brand new objects of the subset type by copying all the information (atlases, vector frames, etc.) contained in <em>A</em> and <em>B</em>.
</p>
TickettscrimTue, 05 Jan 2016 07:55:26 GMT
https://trac.sagemath.org/ticket/18529#comment:108
https://trac.sagemath.org/ticket/18529#comment:108
<p>
There are 2 really, really good ideas for having a more complicated class hierarchy: encapsulation and localization.
</p>
<p>
You had many <code>if self._manifold is self:</code> statements, which breaks the localization principle; it is acting differently because it is essentially a different object. This is a strong code smell and you had enough of these at this level to (IMO) warrant splitting the classes.
</p>
<p>
Additionally, subsets of a manifold are precisely that, subsets. So we should encapsulate that in separate classes. Mathematically we identity an open subset as a manifold, but by doing so, we loose the information that it is really a subset of another manifold. As such, I feel that having separate classes actually better reflects the mathematics.
</p>
<p>
In an ideal world, I would like to have a hierarchy similar to Hierachy2, but with an additional subclass corresponding to each type of manifold that is obtained by adding a subset mixin class. Yet, that doesn't seem possible with the current core design.
</p>
<p>
While the Hierarchy1 seems more complicated, it actually will be easier to maintain and debug as code is local to the class (i.e., if you are a subclass of <code>A</code>, then you act in the same way and like <code>A</code> irregardless of your input).
</p>
<p>
This assessment is based on my experiences and what I learned as good OOP principles. However, I have not looked at SageManifolds beyond this ticket at present, whereas you have worked heavily on it. So perhaps we can get some more data. How many times before the refactoring did you have to do a <code>self._manifold is self</code> test? Also what is your thoughts on maintenance and extendability?
</p>
<p>
Something else might be to get a 3rd party's opinion (Nicolas and/or Darij comes to my mind). I could also sit down and plan out how I would do everything from the ground up to see if I can devise a better overall strategy if you think there might be some benefit to that at this stage.
</p>
TicketegourgoulhonTue, 05 Jan 2016 14:20:07 GMT
https://trac.sagemath.org/ticket/18529#comment:109
https://trac.sagemath.org/ticket/18529#comment:109
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:108" title="Comment 108">tscrim</a>:
</p>
<blockquote class="citation">
<p>
You had many <code>if self._manifold is self:</code> statements, which breaks the localization principle;
</p>
</blockquote>
<p>
Well not so many: grep returns only 5 occurences in the whole SageManifolds:
</p>
<ul><li>in <code>TopologicalManifold._repr_</code> for returning "Manifold..." instead of "Open subset of manifold..."
</li><li>in <code>DifferentiableManifold._repr_</code> for the same reason
</li><li>in <code>ManifoldSubset.superset</code>, because a superset can only be <code>self</code> in case of the whole manifold
</li><li>in <code>ManifoldSubset.intersection</code>, because the intersection with the whole manifold is trivial
</li><li>in <code>ManifoldSubset.union</code>, because the union with the whole manifold is trivial
</li></ul><p>
</p>
<blockquote class="citation">
<p>
Additionally, subsets of a manifold are precisely that, subsets. So we should encapsulate that in separate classes. Mathematically we identity an open subset as a manifold, but by doing so, we loose the information that it is really a subset of another manifold. As such, I feel that having separate classes actually better reflects the mathematics.
</p>
</blockquote>
<p>
Actually, at the moment, we are dealing with the open subsets in exactly the same way as we are doing with the whole manifold, i.e. we set up charts on them, define tensor fields, etc. This can be seen by noticing that in Hierarchy1, the class <code>OpenTopSubmanifold</code> has very few methods in addition to those inherited from <code>TopologicalManifold</code>. Similarly, none of the classes <code>OpenDiffSubmanifold</code> and <code>DifferentiableManifold</code> introduces any attribute or method by itself, in addition to those inherited from <code>DifferentiableMixin</code> (except for their <code>__init__</code>).
Maybe, in a future development, one could have operations that make sense only for open subsets or only for the whole manifold. Then having two separate classes is clearly the way to go. However, at the moment I don't see any such operation, apart from those listed above: the printout (via <code>_repr_</code>) and the union/intersection with other subsets.
</p>
<blockquote class="citation">
<p>
In an ideal world, I would like to have a hierarchy similar to Hierachy2, but with an additional subclass corresponding to each type of manifold that is obtained by adding a subset mixin class. Yet, that doesn't seem possible with the current core design.
</p>
<p>
While the Hierarchy1 seems more complicated, it actually will be easier to maintain and debug as code is local to the class (i.e., if you are a subclass of <code>A</code>, then you act in the same way and like <code>A</code> irregardless of your input).
</p>
<p>
This assessment is based on my experiences and what I learned as good OOP principles. However, I have not looked at SageManifolds beyond this ticket at present, whereas you have worked heavily on it. So perhaps we can get some more data. How many times before the refactoring did you have to do a <code>self._manifold is self</code> test?
</p>
</blockquote>
<p>
Five times, as mentioned above.
</p>
<blockquote class="citation">
<p>
Also what is your thoughts on maintenance and extendability?
</p>
</blockquote>
<p>
It seems to me that, in terms of maintenance, Hierarchy2 is better, being simpler; it is also certainly better for somebody new entering into the code.
On the contrary, if one regards extendability, Hierarchy1 offers the possibility to have different operations for manifolds and subsets, as mentioned above. This is a good point in favor of it. Even if at the moment I don't have in mind any such operation (apart from those listed above), I would slightly tend to maintain Hierarchy1 because of this.
</p>
<blockquote class="citation">
<p>
Something else might be to get a 3rd party's opinion (Nicolas and/or Darij comes to my mind).
</p>
</blockquote>
<p>
Yes extra points of view would be helpful! In addition to Nicolas and Darij, I don't know if other people associated to this ticket (Vincent, John, Nils, Jeoren,...) are reading this and have an opinion...
</p>
<blockquote class="citation">
<p>
I could also sit down and plan out how I would do everything from the ground up to see if I can devise a better overall strategy if you think there might be some benefit to that at this stage.
</p>
</blockquote>
<p>
Thanks, but maybe this is not necessary at this stage, especially if we conclude that we maintain Hierarchy1. Since all this has little impact on the user interface, maybe we could rediscuss it after having a whole view of all the manifold tickets.
</p>
TicketegourgoulhonTue, 05 Jan 2016 15:34:21 GMT
https://trac.sagemath.org/ticket/18529#comment:110
https://trac.sagemath.org/ticket/18529#comment:110
<p>
Giving a second thought to this, another hierarchy that would preserve the distinction between whole manifolds and open subsets is
</p>
<pre class="wiki">Hierarchy3:
AbstractAmbient ManifoldSubset
   
   OpenTopSubmanifold
  \ / 
  TopologicalManifold 
  
  OpenDiffSubmanifold
 \ / 
 DifferentiableManifold 
 
 OpenSubinterval
\____ ____/
OpenInterval

RealLine
</pre><p>
The class <code>AbstractAmbient</code> would only implement the methods <code>union</code> and <code>intersection</code>, which are trivial in this case. Each of the classes <code>TopologicalManifold</code>, <code>DifferentiableManifold</code> and <code>OpenInterval</code> would implement only the method <code>_repr_</code>.
</p>
<p>
Hierarchy3 is simpler than Hierarchy1 and does not require any mixin class.
It is also easy to add a new structure, like complex manifolds.
Hierarchy3 is also mathematically neat, since a topological (resp. differentiable) manifold is obviously a open subset of a topological (resp. differentiable) manifold. In this respect it reverses the logic of Hierarchy1, where the class <code>OpenTopSubmanifold</code> inherits from <code>TopologicalManifold</code>, not the opposite. Maybe the latter logic is quite well spread for <em>algebraic</em> structures in Sage, I mean classes for substructures inheriting from classes for the ambient structure. But for <em>topology</em>, the reverse logic, as proposed in Hierarchy3, could be more adapted: a topological space is often treated as an open subset of itself. For instance, this occurs in its very definition: a topological space is a set X endowed with a collection of subsets of X, called the open subsets, such that the empty set and X are open, etc.
What do you think?
</p>
TicketegourgoulhonFri, 08 Jan 2016 10:37:03 GMT
https://trac.sagemath.org/ticket/18529#comment:111
https://trac.sagemath.org/ticket/18529#comment:111
<p>
Here is another proposal, which is a kind of mix between Hierarchy1 and Hierarchy3:
</p>
<pre class="wiki">
Hierarchy4:
AbstractAmbient AbstractSet AbstractSubset
    \ /   
    ManifoldSubset   
      
   TopologicalManifoldOpenSet   
  \ /  \ /  
  TopologicalManifold  OpenTopSubmanifold  
    
  DifferentiableManifoldOpenSet  
 \ /  \ / 
 DifferentiableManifold  OpenDiffSubmanifold 
  
 IntervalOpenSet /
\____ ____/ \ __________/
OpenInterval OpenSubInterval

RealLine
</pre><p>
Contrary to Hierarchy1, it has no diamond problem and involves no mixin class.
As in Hierarchy3, the class <code>AbstractAmbient</code> implements only the methods <code>union</code> and <code>intersection</code>, but contrary to Hierarchy3, the classes for ambient objects (<code>TopologicalManifold</code>, <code>DifferentiableManifold</code>, etc.) do no longer have the attribute <code>_manifold</code>. This attribute is present only in the subobject classes <code>OpenTopSubmanifold</code>, <code>OpenDiffSubmanifold</code> and <code>OpenSubinterval</code>, as in Hierarchy1. In Hierarchy4, the attribute <code>_manifold</code> is inherited from <code>AbstractSubset</code>. The class <code>ManifoldSubset</code> implements generic (not open) subsets of a manifold.
The classes <code>TopologicalManifold</code> and <code>OpenTopSubmanifold</code> do not implement
any new method by themselves: most methods are implemented in their parent
abstract class <code>TopologicalManifoldOpenSet</code>, like <code>chart()</code>, <code>atlas()</code>,
<code>scalar_field_algebra()</code>, etc.
Similarly, methods for differentiable manifolds, like <code>vector_frame()</code> or
<code>tensor_field()</code>, are implemented in the abstract class <code>DifferentiableManifoldOpenSet</code>.
</p>
TickettscrimSun, 10 Jan 2016 03:43:04 GMT
https://trac.sagemath.org/ticket/18529#comment:112
https://trac.sagemath.org/ticket/18529#comment:112
<p>
I think if we are really going to go through with what might be some hard work and separate out the subset features, we should just do this:
</p>
<pre class="wiki"> AbstractSet AbstractSubsetMixin
   
 /  
TopologicalManifold /  
 \_____ / / 
 \ / / 
 TopologicalSubmanifold / 
DifferentiableManifold / /
 \__ / /
 \ / /
 DifferentiableSubmanifold /
OpenInterval /
 \__________ ____________/
 \ /
 OpenSubinterval
RealLine
</pre><p>
It is what I was hoping to obtain, but had trouble separating out the subset portion because there still is a small diamond problem with <code>Parent</code>. Which is why <code>AbstractSubset</code> would have to be a mixin, both in this hierarchy and Hierarchy3,4 to be effective.
</p>
<p>
It does seem like perhaps Hierarchy2 would be the best since there is so few tests for <code>if self._manifold is self</code>. The biggest trouble I have with this is the separations of concerns: that it makes it somewhat harder to separate these classes later if they start needing different attributes. We also have wellestablished code in Sage for which there is a different class for subobjects.
</p>
<p>
Ah hell, perhaps we should just revert back to Hierarchy2 to keep the simplicity. I'm starting to wonder if there is perhaps a more fundamental issue in that we are trying to be too close to the mathematical definitions rather than be programmers, which could potentially be a complete rewrite of most things. However, we have working code, which is always better than no code. Anyways, I'm going to shutup now and just ask do you want me to handle the revert or are you willing to do it?
</p>
TicketegourgoulhonSun, 10 Jan 2016 14:29:53 GMT
https://trac.sagemath.org/ticket/18529#comment:113
https://trac.sagemath.org/ticket/18529#comment:113
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:112" title="Comment 112">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It is what I was hoping to obtain, but had trouble separating out the subset portion because there still is a small diamond problem with <code>Parent</code>. Which is why <code>AbstractSubset</code> would have to be a mixin, both in this hierarchy and Hierarchy3,4 to be effective.
</p>
</blockquote>
<p>
Yes. Similarly <code>AbstractAmbient</code> in Hierarchy4 has to be a mixin, so that only <code>AbstractSet</code> inheritates from <code>Parent</code>.
</p>
<blockquote class="citation">
<p>
It does seem like perhaps Hierarchy2 would be the best since there is so few tests for <code>if self._manifold is self</code>. The biggest trouble I have with this is the separations of concerns: that it makes it somewhat harder to separate these classes later if they start needing different attributes. We also have wellestablished code in Sage for which there is a different class for subobjects.
</p>
</blockquote>
<p>
As said before, it is difficult to see at the monent what kind of different attributes would exist for manifolds and open submanifolds. Basically, our open submanifolds behave just like manifolds, especially when defining fields on them. This reflects the fact that both are open sets, which is the true basic structure in topology. Things are probably different in other parts of Sage, which deal with algebraic structures, instead of topological ones.
</p>
<blockquote class="citation">
<p>
Ah hell, perhaps we should just revert back to Hierarchy2 to keep the simplicity. I'm starting to wonder if there is perhaps a more fundamental issue in that we are trying to be too close to the mathematical definitions rather than be programmers, which could potentially be a complete rewrite of most things. However, we have working code, which is always better than no code.
</p>
</blockquote>
<p>
OK, let us revert to Hierarchy2. It is simple, effective, offers a better gathering of documentation, and, via <code>self._manifold = self</code>, it embodies the fact that a manifold is an open subset of itself, which is not a totally crazy mathematical idea.
Anyway, the whole discussion has been very interesting (at least to me) and we can keep the above diagrams for future thoughts.
</p>
<blockquote class="citation">
<p>
Anyways, I'm going to shutup now and just ask do you want me to handle the revert or are you willing to do it?
</p>
</blockquote>
<p>
I will do it. Of course, it will not be a complete revert to the state prior to your refactorization: we shall keep your singleton classes for the structure (topological, differentiable, etc.) and your removal of facade parents, which are nice features.
</p>
TicketgitSun, 10 Jan 2016 22:28:09 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:114
https://trac.sagemath.org/ticket/18529#comment:114
<ul>
<li><strong>commit</strong>
changed from <em>3cd03a48d847e12745ed8c25b23f19db141c179a</em> to <em>984c3c26bf827f44eee26fc6afd321e11dca8f2e</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=984c3c26bf827f44eee26fc6afd321e11dca8f2e"><span class="icon"></span>984c3c2</a></td><td><code>Revert to simple hierarchy for manifold classes</code>
</td></tr></table>
TicketegourgoulhonSun, 10 Jan 2016 22:37:02 GMTstatus changed
https://trac.sagemath.org/ticket/18529#comment:115
https://trac.sagemath.org/ticket/18529#comment:115
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
Following <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:113" title="Comment 113">comment:113</a>, the above commit reverts to Hierarchy2 (only the first two items of it, <code>ManifoldSubset</code> and <code>TopologicalManifold</code> are implemented in this ticket).
</p>
TicketegourgoulhonSun, 10 Jan 2016 22:45:10 GMTdescription changed
https://trac.sagemath.org/ticket/18529#comment:116
https://trac.sagemath.org/ticket/18529#comment:116
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/18529?action=diff&version=116">diff</a>)
</li>
</ul>
TickettscrimTue, 12 Jan 2016 04:17:36 GMT
https://trac.sagemath.org/ticket/18529#comment:117
https://trac.sagemath.org/ticket/18529#comment:117
<p>
I also think this discussion was good. I'm just slightly frustrated at myself for not being able to come up with a clear better alternative to get rid of the code smell.
</p>
<p>
Anyways, now it is back to going over the documentation and technicalities of the code. I hope to finish this review in a couple of days. I'm also thinking that the followup tickets will probably be a lot easier/faster to review too.
</p>
TicketegourgoulhonTue, 12 Jan 2016 12:32:07 GMT
https://trac.sagemath.org/ticket/18529#comment:118
https://trac.sagemath.org/ticket/18529#comment:118
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:117" title="Comment 117">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I also think this discussion was good. I'm just slightly frustrated at myself for not being able to come up with a clear better alternative to get rid of the code smell.
</p>
</blockquote>
<p>
I think there is still the opportunity to implement another hierarchy latter, if we feel that specificities for subsets are required. Also, if we do this latter, when more tickets of <a class="new ticket" href="https://trac.sagemath.org/ticket/18528" title="task: SageManifolds metaticket (new)">#18528</a> are merged, this would alleviate the propagation of the changes to other tickets and one would notice at once possible side effects.
</p>
<blockquote class="citation">
<p>
Anyways, now it is back to going over the documentation and technicalities of the code. I hope to finish this review in a couple of days.
</p>
</blockquote>
<p>
Thanks.
</p>
<blockquote class="citation">
<p>
I'm also thinking that the followup tickets will probably be a lot easier/faster to review too.
</p>
</blockquote>
<p>
I've propagated the latest changes in this ticket to all tickets of <a class="new ticket" href="https://trac.sagemath.org/ticket/18528" title="task: SageManifolds metaticket (new)">#18528</a>. Everything is OK. In particular, the new singleton classes for manifold structures have allowed to simplify the code, avoiding to redefine the methods <code>chart</code> and <code>scalar_field_algebra</code> in the class <code>DifferentiableManifold</code>.
</p>
TicketgitSun, 24 Jan 2016 01:14:55 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:119
https://trac.sagemath.org/ticket/18529#comment:119
<ul>
<li><strong>commit</strong>
changed from <em>984c3c26bf827f44eee26fc6afd321e11dca8f2e</em> to <em>4ed37cece6c44c8c55e79b4dd9990eafe29c20d9</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=af727a804f88983805f5518242ce21edd8ae45cb"><span class="icon"></span>af727a8</a></td><td><code>Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=1113442b63aa24c397fc2f5755402cbb061b4654"><span class="icon"></span>1113442</a></td><td><code>Some final reviewer changes and tweaks.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=4ed37cece6c44c8c55e79b4dd9990eafe29c20d9"><span class="icon"></span>4ed37ce</a></td><td><code>Some documentation rewording and other small tweaks.</code>
</td></tr></table>
TickettscrimSun, 24 Jan 2016 01:23:06 GMTreviewer, milestone changed
https://trac.sagemath.org/ticket/18529#comment:120
https://trac.sagemath.org/ticket/18529#comment:120
<ul>
<li><strong>reviewer</strong>
changed from <em>Travis Scrimshaw</em> to <em>Travis Scrimshaw, Eric Gourgoulhon</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage7.0</em> to <em>sage7.1</em>
</li>
</ul>
<p>
Okay, I've made my last pass through everything. I have two comments left:
</p>
<ul><li>In <code>Chart.set_inverse</code>, there is a keyword <code>check</code> that I changed to <code>verbose</code> because there was no check being done. Irregardless, I don't quite like that the default is to print things to the terminal. Is there a specific reason for this? I feel like the default should be <code>False</code>.
</li><li>In <code>ManifoldPoint</code>, the methods <code>set_coord</code> should be the full <code>set_coordinates</code> (and similar methods). The short version can be an alias. (Same discussion as for <code>disp</code> vs <code>display</code>.).
</li></ul><p>
Once we take care of these, then this will be a positive review.
</p>
TicketgitSun, 24 Jan 2016 01:25:36 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:121
https://trac.sagemath.org/ticket/18529#comment:121
<ul>
<li><strong>commit</strong>
changed from <em>4ed37cece6c44c8c55e79b4dd9990eafe29c20d9</em> to <em>cf5e98b2041d1fdee0756e99a85ada0b865375da</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=cf5e98b2041d1fdee0756e99a85ada0b865375da"><span class="icon"></span>cf5e98b</a></td><td><code>Use standard copyright template.</code>
</td></tr></table>
TicketgitSun, 24 Jan 2016 20:00:43 GMTcommit changed
https://trac.sagemath.org/ticket/18529#comment:122
https://trac.sagemath.org/ticket/18529#comment:122
<ul>
<li><strong>commit</strong>
changed from <em>cf5e98b2041d1fdee0756e99a85ada0b865375da</em> to <em>00d265cf2855121dba914868264da6ea3a9c42af</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=0c9d7fab348b69615cbfc065824c2c3b5fd62fbc"><span class="icon"></span>0c9d7fa</a></td><td><code>Merge branch 'public/manifolds/top_manif_basics' into Sage 7.1.beta0.</code>
</td></tr><tr><td><a class="extlink" href="http://git.sagemath.org/sage.git/commit/?id=00d265cf2855121dba914868264da6ea3a9c42af"><span class="icon"></span>00d265c</a></td><td><code>Set verbose=False as the default in CoordChange.set_inverse; change "coord" to "coordinates" in names of ManifoldPoint methods.</code>
</td></tr></table>
TicketegourgoulhonSun, 24 Jan 2016 20:10:46 GMT
https://trac.sagemath.org/ticket/18529#comment:123
https://trac.sagemath.org/ticket/18529#comment:123
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18529#comment:120" title="Comment 120">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Okay, I've made my last pass through everything.
</p>
</blockquote>
<p>
Thanks a lot for all your changes.
</p>
<blockquote class="citation">
<p>
I have two comments left:
</p>
<ul><li>In <code>Chart.set_inverse</code>, there is a keyword <code>check</code> that I changed to <code>verbose</code> because there was no check being done.
</li></ul></blockquote>
<p>
Yes, <code>verbose</code> seems more appropriate.
</p>
<blockquote class="citation">
<p>
Irregardless, I don't quite like that the default is to print things to the terminal. Is there a specific reason for this?
</p>
</blockquote>
<p>
No.
</p>
<blockquote class="citation">
<p>
I feel like the default should be <code>False</code>.
</p>
</blockquote>
<p>
I've set it to <code>False</code> in the above commit.
</p>
<blockquote class="citation">
<ul><li>In <code>ManifoldPoint</code>, the methods <code>set_coord</code> should be the full <code>set_coordinates</code> (and similar methods). The short version can be an alias. (Same discussion as for <code>disp</code> vs <code>display</code>.).
</li></ul></blockquote>
<p>
I've performed the change in the above commit, adding aliases for short versions of <code>coordinates</code>, <code>add_coordinates</code> and <code>set_coordinates</code>. I've also corrected one or two typos.
</p>
TickettscrimSun, 24 Jan 2016 20:15:34 GMTstatus changed
https://trac.sagemath.org/ticket/18529#comment:124
https://trac.sagemath.org/ticket/18529#comment:124
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
LGTM. Here is the start of the main body of SageManifolds into Sage. Thank you for all your hard work.
</p>
TicketegourgoulhonSun, 24 Jan 2016 20:56:46 GMT
https://trac.sagemath.org/ticket/18529#comment:125
https://trac.sagemath.org/ticket/18529#comment:125
<p>
Thanks a lot for the detailed review work, the numerous improvements and the enlightening discussions!
</p>
TicketvbraunThu, 28 Jan 2016 17:14:31 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/18529#comment:126
https://trac.sagemath.org/ticket/18529#comment:126
<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/top_manif_basics</em> to <em>00d265cf2855121dba914868264da6ea3a9c42af</em>
</li>
</ul>
Ticket