Sage: Ticket #9650: Adding support for differential forms
https://trac.sagemath.org/ticket/9650
<p>
Implements support for differential forms to Sage. After applying the patch, three new classes are added to sage:
</p>
<ol><li><code>CoordinatePatch</code> -- an open subset of Rn on which differential forms can be defined
</li><li><code>DifferentialForms</code> -- a differential forms parent
</li><li><code>DifferentialForm</code> -- a differential forms class.
</li></ol><p>
Please see the documentation in the <code>DifferentialForm</code> class for more information about syntax, available methods, etc.
</p>
<p>
This is a very basic implementation, providing support for exterior differentiation and wedge products of forms, but not much more. The emphasis is on making sure that the framework is implemented correctly.
</p>
<p>
See discussion at
</p>
<ol><li><a class="ext-link" href="http://wiki.sagemath.org/tensorcalc"><span class="icon"></span>http://wiki.sagemath.org/tensorcalc</a>
</li><li><a class="ext-link" href="http://groups.google.be/group/sage-devel/browse_thread/thread/2feef1f0be557585/c2b7095747ebe34d"><span class="icon"></span>http://groups.google.be/group/sage-devel/browse_thread/thread/2feef1f0be557585/c2b7095747ebe34d</a>
</li></ol>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/9650
Trac 1.1.6jvkerschSat, 31 Jul 2010 12:09:41 GMTcc set
https://trac.sagemath.org/ticket/9650#comment:1
https://trac.sagemath.org/ticket/9650#comment:1
<ul>
<li><strong>cc</strong>
<em>jason</em> added
</li>
</ul>
TicketjasonSat, 31 Jul 2010 18:30:05 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:2
https://trac.sagemath.org/ticket/9650#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_work</em>
</li>
</ul>
<p>
Wow, thanks! I'm not an expert in differential geometry, so I'm going to have to rely on someone else to vet the theoretical design at this level. Here are a few python comments, though:
</p>
<ul><li><code>all([is_SymbolicVariable(c) for c in coordinates])</code> should not construct a list, so that short-circuiting can occur: <code>all(is_SymbolicVariable(c) for c in coordinates)</code>
</li></ul><ul><li>Checking for <code>None</code> should be done with is (it's a lot faster that way): <code>metric is not None</code>
</li></ul><p>
I also added mention of two other mma packages to the wiki page, one of which has a nice Integral command. Do you see us getting a command that can integrate like the following commands indicate?
</p>
<pre class="wiki">The area of the unit square is calculated by:
Integral[ d[x,y] , Chain[ {x -> s, y -> t}, {s, 0, 1}, {t, 0, 1}]].
The area of the circle of radius R is calculated by:
SetAttributes[R, Constant];
Integral[ d[x,y] , Chain[ {x -> r Cos[theta], y -> r Sin[theta]}, \
{r, 0, R}, {theta, 0, 2Pi}]].
Stokes Theorem:
Integral[ d @ ((x/2) d[y] - (y/2) d[y]) , Chain[ {x -> s, y -> t}, \
{s, 0, 1}, {t, 0, 1}]] ==
Integral[ ((x/2) d[y] - (y/2) d[y]) , Boundary @ Chain[ {x -> s, y -> \
t}, {s, 0, 1}, {t, 0, 1}]]
</pre>
TicketjasonSat, 31 Jul 2010 18:30:37 GMT
https://trac.sagemath.org/ticket/9650#comment:3
https://trac.sagemath.org/ticket/9650#comment:3
<p>
Also, if you feel like the patch is to the point that it needs review, please change the status below to "needs review".
</p>
TicketjasonSat, 31 Jul 2010 18:30:51 GMTmilestone set
https://trac.sagemath.org/ticket/9650#comment:4
https://trac.sagemath.org/ticket/9650#comment:4
<ul>
<li><strong>milestone</strong>
set to <em>sage-4.5.2</em>
</li>
</ul>
TicketjvkerschSun, 01 Aug 2010 16:55:11 GMT
https://trac.sagemath.org/ticket/9650#comment:5
https://trac.sagemath.org/ticket/9650#comment:5
<p>
Thanks for the comments, which not only improved the patch but my Python knowledge as well! I've updated the patch in the meantime. Thanks also for guiding me through the sage patch process!
</p>
<p>
Some quick comments and questions:
</p>
<ol><li> As for integration of forms, I think this is definitely doable and in fact should be dealt with before the other stuff (metric geometry, etc).
</li></ol><ol start="2"><li> I'm very interesting in seeing how the authors of that mathematica package represented differential forms -- thanks for the link!
</li></ol><ol start="3"><li>Once I've taken a good look at the mathematica package, I'll set the patch to "needs review".
</li></ol>
TicketjdemeyerWed, 04 Aug 2010 08:28:56 GMTcc changed
https://trac.sagemath.org/ticket/9650#comment:6
https://trac.sagemath.org/ticket/9650#comment:6
<ul>
<li><strong>cc</strong>
<em>jdemeyer</em> added
</li>
</ul>
TicketjvkerschWed, 04 Aug 2010 08:52:36 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:7
https://trac.sagemath.org/ticket/9650#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I'm going to set the patch to "needs review", firstly since as burcin pointed out in a private email, the licensing situation of the mathematica package is somewhat unclear, and secondly the implementation of the differential forms class is sufficiently simple so that the issue won't be with the algorithms that I used but rather whether this is good sage programming.
</p>
<p>
Some things to keep in mind: right now, the way to create a differential form is as follows: first you create a ``<a class="missing wiki">CoordinatePatch?</a>`` on which forms can be defined, then you create a ``<a class="missing wiki">DifferentialForms?</a>`` parent and then you can create forms. Explicitly, this looks like
</p>
<pre class="wiki">
sage: x, y, z = var('x, y, z')[[BR]] sage: U = !CoordinatePatch((x, y, z))[[BR]] sage: F = !DifferentialForms(U)[[BR]] sage: form = !DifferentialForm(F, 0, sin(x*y)); form[[BR]] sin(x*y)
</pre><p>
Let me know if this construction is confusing or can be simplified in any way.
</p>
TicketjasonWed, 04 Aug 2010 14:42:40 GMT
https://trac.sagemath.org/ticket/9650#comment:8
https://trac.sagemath.org/ticket/9650#comment:8
<p>
Oops: inside of triple-braces, things are quoted literally:
</p>
<pre class="wiki">sage: x, y, z = var('x, y, z')
sage: U = CoordinatePatch((x, y, z))
sage: F = DifferentialForms(U)
sage: form = DifferentialForm(F, 0, sin(x*y)); form
sin(x*y)
</pre>
TicketnovoseltFri, 13 Aug 2010 02:34:07 GMTcc changed
https://trac.sagemath.org/ticket/9650#comment:9
https://trac.sagemath.org/ticket/9650#comment:9
<ul>
<li><strong>cc</strong>
<em>novoselt</em> added
</li>
</ul>
TicketjvkerschSun, 22 Aug 2010 13:57:47 GMTauthor changed
https://trac.sagemath.org/ticket/9650#comment:10
https://trac.sagemath.org/ticket/9650#comment:10
<ul>
<li><strong>author</strong>
changed from <em>jvkersch</em> to <em>Joris Vankerschaver</em>
</li>
</ul>
TicketmpatelSun, 22 Aug 2010 18:44:06 GMTcc changed
https://trac.sagemath.org/ticket/9650#comment:11
https://trac.sagemath.org/ticket/9650#comment:11
<ul>
<li><strong>cc</strong>
<em>mpatel</em> added
</li>
</ul>
TicketmhamptonMon, 23 Aug 2010 00:47:04 GMTcc changed
https://trac.sagemath.org/ticket/9650#comment:12
https://trac.sagemath.org/ticket/9650#comment:12
<ul>
<li><strong>cc</strong>
<em>mhampton</em> added
</li>
</ul>
TicketmpatelMon, 23 Aug 2010 01:19:32 GMT
https://trac.sagemath.org/ticket/9650#comment:13
https://trac.sagemath.org/ticket/9650#comment:13
<p>
To placate <code>./sage -b</code>, which now gives
</p>
<pre class="wiki">package init file 'sage/tensor/__init__.py' not found (or not a regular file)
</pre><p>
messages, could you add an empty <code>sage/tensor/__init__.py</code>.
</p>
<p>
Also, if you'd like to document your new modules in the reference manual, you can add <code>tensor</code> to
</p>
<pre class="wiki">SAGE_ROOT/devel/sage/doc/en/reference/index.rst
</pre><p>
and create <code>tensor.rst</code>, patterned after <code>graphs.rst</code> (say), in the same directory. To rebuild the manual, run
</p>
<div class="wiki-code"><div class="code"><pre><span class="nv">$ </span><span class="nb">cd </span>SAGE_ROOT
<span class="nv">$ </span>./sage -b
<span class="nv">$ </span>./sage -docbuild reference html -j
</pre></div></div><p>
Sphinx should print warnings about any docstring formatting problems. The results will be in
</p>
<pre class="wiki">SAGE_ROOT/devel/sage/doc/output/html/en/reference/
</pre>
TicketnilesMon, 23 Aug 2010 16:53:10 GMTstatus, cc changed; reviewer, work_issues set
https://trac.sagemath.org/ticket/9650#comment:14
https://trac.sagemath.org/ticket/9650#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>Niles Johnson</em>
</li>
<li><strong>work_issues</strong>
set to <em>__init__.py, documentation warnings, gens()</em>
</li>
<li><strong>cc</strong>
<em>niles</em> added
</li>
</ul>
<p>
Hi Joris,
</p>
<p>
I saw your e-mail to <code>sage-devel</code>, so here's my review :) This looks like a nice patch, and it works nicely enough that I had to resist the temptation to ask for more functionality (which you are right to postpone, I think).
</p>
<p>
I ran a complete doctest of the sage library, and tested building the documentation, as well as playing with a few examples. Here are some notes
</p>
<ul><li>add empty file <span class="underline">init</span>.py (as mentioned above; this is required for doctesting and to build documentation)
</li><li>add author to coordinate_patch.py
</li><li>I believe it is usual for gens() to return a tuple (rather than a list); e.g.
<pre class="wiki">sage: PolynomialRing(RR,3,'x,y,z').gens()
(x, y, z)
</pre></li></ul><ul><li><code>make ptestlong</code> passes all tests!
</li></ul><ul><li>Following the above suggestion, I added 'tensor' to <code>index.rst</code> and the following to a new file <code>tensor.rst</code>. There are a few warnings when building the documentation that need to be fixed, but mostly it looks good.
<pre class="wiki">Differential Forms
============
.. toctree::
:maxdepth: 2
sage/tensor/coordinate_patch
sage/tensor/differential_forms
sage/tensor/differential_form_element
</pre></li></ul><ul><li>Is there a reason you call the directory <code>tensor</code> instead of e.g. <code>diff_forms</code> or even <code>differential_forms</code>? I'm not an expert on differential forms, so I defer to your judgment, but the latter seems more intuitive to me.
</li></ul>
TicketnilesMon, 23 Aug 2010 16:54:55 GMT
https://trac.sagemath.org/ticket/9650#comment:15
https://trac.sagemath.org/ticket/9650#comment:15
<p>
the first point is about <code>__init__.py</code>; sorry for the misformatting!
</p>
TicketnilesMon, 23 Aug 2010 17:08:17 GMT
https://trac.sagemath.org/ticket/9650#comment:16
https://trac.sagemath.org/ticket/9650#comment:16
<p>
oh, and doctest coverage is at 100% for each of the 3 files!
</p>
TicketjvkerschTue, 24 Aug 2010 12:44:39 GMT
https://trac.sagemath.org/ticket/9650#comment:17
https://trac.sagemath.org/ticket/9650#comment:17
<p>
Hi Niles and Mitesh,
</p>
<p>
Thank you for your instructive comments. Niles, thanks also for agreeing to review my patch! The suggestion to include this functionality to the reference manual was especially helpful -- it makes everything so much clearer to see the documentation in nice, crisp HTML form.
</p>
<p>
I implemented the changes you suggested (adding to the reference manual, adding authors, making <code>gens()</code> return a tuple), but I have a few questions/comments that are more of a technical nature, revealing simultaneously my mercurial ineptitude:
</p>
<ol><li> I made a mess of the upload section -- could someone with admin privileges delete all but the most recent attachment?
</li></ol><ol start="2"><li> Try as I might, I could not produce a patch which creates the file <code>__init__.py</code>. I added that file to my sage tree as per the documentation, instructed hg to add it, confirmed with <code>hg status</code> that it was listed as added, but when I look at <code>hg diff</code> that file is nowhere to be found. The other files are created/modified as expected. The patch also does not create <code>__init__.py</code>, as you experienced. Are these init files somehow special as far as hg is concerned?
</li></ol><ol start="3"><li> I called the package <code>tensor</code> rather than anything more specific in order to accommodate future additions: it would be good to have support for generic tensors (not just differential forms) and common operations on them (e.g. covariant derivations), so that we could for instance do symbolic Riemannian geometry. However, if you think we should stick to a more specific name for now, then that's fine with me too.
</li></ol>
TicketjasonTue, 24 Aug 2010 14:23:26 GMT
https://trac.sagemath.org/ticket/9650#comment:18
https://trac.sagemath.org/ticket/9650#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:17" title="Comment 17">jvkersch</a>:
</p>
<blockquote class="citation">
<p>
Hi Niles and Mitesh,
</p>
<p>
Thank you for your instructive comments. Niles, thanks also for agreeing to review my patch! The suggestion to include this functionality to the reference manual was especially helpful -- it makes everything so much clearer to see the documentation in nice, crisp HTML form.
</p>
<p>
I implemented the changes you suggested (adding to the reference manual, adding authors, making <code>gens()</code> return a tuple), but I have a few questions/comments that are more of a technical nature, revealing simultaneously my mercurial ineptitude:
</p>
<ol><li> I made a mess of the upload section -- could someone with admin privileges delete all but the most recent attachment?
</li></ol></blockquote>
<p>
Done.
</p>
<blockquote class="citation">
<ol start="2"><li> Try as I might, I could not produce a patch which creates the file <code>__init__.py</code>. I added that file to my sage tree as per the documentation, instructed hg to add it, confirmed with <code>hg status</code> that it was listed as added, but when I look at <code>hg diff</code> that file is nowhere to be found. The other files are created/modified as expected. The patch also does not create <code>__init__.py</code>, as you experienced. Are these init files somehow special as far as hg is concerned?
</li></ol></blockquote>
<p>
A peculiarity of Mercurial is that it can't check in empty files. So usually we'll add either a space, or a comment like:
</p>
<pre class="wiki"># This comment is here so the file is non-empty (so Mercurial will check it in).
</pre><p>
or something like that.
</p>
<blockquote class="citation">
<ol start="3"><li> I called the package <code>tensor</code> rather than anything more specific in order to accommodate future additions: it would be good to have support for generic tensors (not just differential forms) and common operations on them (e.g. covariant derivations), so that we could for instance do symbolic Riemannian geometry. However, if you think we should stick to a more specific name for now, then that's fine with me too.
</li></ol></blockquote>
<p>
Personally, I'm okay with "tensor", since "differential_forms" would be equally confusing to my calc 3 students, so (a) there will be a learning curve anyway, and (b) most functions students would use are probably going to be imported into the top-level namespace anyway.
</p>
TicketjvkerschTue, 24 Aug 2010 19:47:06 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:19
https://trac.sagemath.org/ticket/9650#comment:19
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:18" title="Comment 18">jason</a>:
</p>
<blockquote class="citation">
<p>
A peculiarity of Mercurial is that it can't check in empty files. So usually we'll add either a space, or a comment like:
(...)
</p>
</blockquote>
<p>
Thanks a lot for pointing that out -- this is really helpful!
</p>
<p>
I've updated the patch with the <code>__init__.py</code>, verified that it passes all doctests, and added those files to the reference manual section. So I'm changing the status back to <code>needs_review</code>.
</p>
TicketjasonTue, 24 Aug 2010 21:27:14 GMT
https://trac.sagemath.org/ticket/9650#comment:20
https://trac.sagemath.org/ticket/9650#comment:20
<p>
It looks like the top of coordinate_patch.py is repeated around line 232. Was that intentional?
</p>
TicketjasonTue, 24 Aug 2010 21:28:55 GMT
https://trac.sagemath.org/ticket/9650#comment:21
https://trac.sagemath.org/ticket/9650#comment:21
<p>
In fact, it looks like the entire content of coordinate_patch.py is duplicated, starting around line 232 of that file.
</p>
TicketnilesWed, 25 Aug 2010 15:00:57 GMTstatus, work_issues changed
https://trac.sagemath.org/ticket/9650#comment:22
https://trac.sagemath.org/ticket/9650#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
changed from <em>__init__.py, documentation warnings, gens()</em> to <em>documentation formatting, duplicates in coordinate_patch.py</em>
</li>
</ul>
<p>
Hi Joris,
</p>
<p>
Things are looking good, but the documentation produces some warnings related to formatting errors--please fix this in addition to deleting the duplicates from <code>coordinate_patch.py</code>. There are also some formatting problems in the documentation which do not produce warnings, so please check the html documentation carefully.
</p>
<p>
Also, do you think the documentation files could have better names? I find it funny that the documentation for "<code>DifferentialForm</code>" is titled "Differential forms class" and the documentation for "<code>DifferentialForms</code>" is titled something else. (But again I'll defer to you on this :)
</p>
TicketjvkerschMon, 30 Aug 2010 21:52:58 GMT
https://trac.sagemath.org/ticket/9650#comment:23
https://trac.sagemath.org/ticket/9650#comment:23
<p>
Hi Jason and Niles,
</p>
<p>
Thanks for your comments, in particular for catching that code duplication. I've tried to address your suggestions as well as I could, and here are the changes in the most recent version of the patch:
</p>
<ol><li> I've fixed the documentation so that it generates the output without any warnings, and I've also addressed the formatting issues. This proved to be really tricky...
</li></ol><ol start="2"><li> I agree on the names of the documentation files. In the current patch, the documentation now reads: "Algebra of differential forms" for <code>differential_forms.py</code> and "Elements of the algebra of differential forms" for <code>differential_form_element.py</code>. I believe that this is both clearer and more in line with the rest of the Sage docs, but please feel free to correct me on this!
</li></ol><p>
Thanks for all your time on the review!
</p>
TicketjvkerschMon, 30 Aug 2010 21:55:22 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:24
https://trac.sagemath.org/ticket/9650#comment:24
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketmpatelMon, 30 Aug 2010 21:58:16 GMT
https://trac.sagemath.org/ticket/9650#comment:25
https://trac.sagemath.org/ticket/9650#comment:25
<p>
Joris, could you add yourself to the <a class="ext-link" href="http://trac.sagemath.org/sage_trac/wiki/WikiStart#AccountNamesmappedtoRealNames"><span class="icon"></span>account name-real name map</a>, when it's convenient?
</p>
TicketjasonTue, 31 Aug 2010 03:30:18 GMT
https://trac.sagemath.org/ticket/9650#comment:26
https://trac.sagemath.org/ticket/9650#comment:26
<p>
The most recent patch looks good to me. Doctests pass, playing around with some examples seems fine, and docs build. However, as this is not my area, at least one other person should pass off on this (niles?).
</p>
TicketmhamptonTue, 31 Aug 2010 15:02:29 GMT
https://trac.sagemath.org/ticket/9650#comment:27
https://trac.sagemath.org/ticket/9650#comment:27
<p>
This looks good to me too, although I'm not sure if this is desired behavior:
</p>
<pre class="wiki">U = CoordinatePatch((x,y))
F2 = DifferentialForms(U)
q = DifferentialForm(F2,1)
q[0] = -y
q[1] = x
diff(q,y)
</pre><p>
gives
</p>
<pre class="wiki">0
</pre><p>
but it seems like diff(q,y) should give dx/\dy. I'm pretty rusty with my differntial forms though, I'll try to brush up.
</p>
TicketnilesTue, 31 Aug 2010 17:07:31 GMTstatus, work_issues changed
https://trac.sagemath.org/ticket/9650#comment:28
https://trac.sagemath.org/ticket/9650#comment:28
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
changed from <em>documentation formatting, duplicates in coordinate_patch.py</em> to <em>diff()</em>
</li>
</ul>
<p>
I agree; documentation builds cleanly, looks good, and the code works nicely. As Joris has said, this is intentionally a very basic implementation; I think later patches should add functionality and flexibility (e.g. differential forms over other rings, and coercion between forms over different sets of variables, as is done for multivariate polynomial rings)
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:27" title="Comment 27">mhampton</a>:
</p>
<blockquote class="citation">
<p>
This looks good to me too, although I'm not sure if this is desired behavior:
</p>
</blockquote>
<pre class="wiki">U = CoordinatePatch((x,y))
F2 = DifferentialForms(U)
q = DifferentialForm(F2,1)
q[0] = -y
q[1] = x
diff(q,y)
</pre><blockquote class="citation">
<p>
gives
</p>
</blockquote>
<pre class="wiki"> 0
</pre><blockquote class="citation">
<p>
but it seems like diff(q,y) should give dx/\dy. I'm pretty rusty with my differntial forms though, I'll try to brush up.
</p>
</blockquote>
<p>
This problem is caused because <code>diff()</code> first tries to call <code>q.differentiate()</code> and, failing that, coerces <code>q</code> to the symbolic ring and differentiates it there. Adding a <code>differentiate()</code> method which simply calls <code>q.diff()</code> would solve this problem. Depending on what you think is best, you could silently ignore additional arguments to <code>diff</code>, or you could throw an error if additional arguments are present.
</p>
TicketjvkerschThu, 02 Sep 2010 05:37:39 GMT
https://trac.sagemath.org/ticket/9650#comment:29
https://trac.sagemath.org/ticket/9650#comment:29
<p>
Hi Marshall and Niles,
</p>
<p>
Thanks for testing the patch. I never would have expected this behavior, so I'm glad it comes out now, thanks!! I followed Niles' suggestion and added a <code>derivative</code> member function which calls <code>diff</code>. If any additional arguments are specified, it throws an exception since I want to avoid situations like these where one tries to take the derivative of a form with respect to a coordinate. This isn't an intrinsic (coordinate-independent) notion, so it would be better to enforce that in the code as well.
</p>
<p>
More generally, Marshall's issue uncovers something strange having to do with coercion into the symbolic ring:
</p>
<pre class="wiki">sage: x, y = var('x, y')
sage: U = CoordinatePatch((x, y))
sage: F = DifferentialForms(U)
sage: q = DifferentialForm(F, 2)
sage: q[0, 1] = sin(x*y); q
sin(x*y)*dx/\dy
sage: SR(q)
sin(x*y)*dx/\dy
sage: q.parent()
Algebra of differential forms in the variables x, y
sage: SR(q).parent()
Symbolic Ring
</pre><p>
I.e, if I coerce <code>q</code> into the symbolic ring, it still looks like a differential form but its parent is <code>SR</code> and the behavior is completely wrong (wedge and diff members give the wrong results). Shouldn't this kind of coercion raise an error, since this could never be the intended behavior?
</p>
<p>
Let me know what you guys think -- for now I'll leave this as needs_work.
</p>
TicketjasonThu, 02 Sep 2010 06:32:04 GMT
https://trac.sagemath.org/ticket/9650#comment:30
https://trac.sagemath.org/ticket/9650#comment:30
<p>
Think of SR as a wrapper around any python object. For example, continuing from your example above
</p>
<pre class="wiki">sage: p=SR(q)
sage: p.pyobject().parent()
Algebra of differential forms in the variables x, y
</pre><p>
I think it's fine to let the explicit conversion SR(q) work---by default, explicitly converting to SR just wraps the object as above, and I think is supported for every Sage object, whether or not it makes "sense". Do you see someplace that the conversion to an SR object is implicit? That would be a problem.
</p>
TicketjasonThu, 02 Sep 2010 06:34:16 GMT
https://trac.sagemath.org/ticket/9650#comment:31
https://trac.sagemath.org/ticket/9650#comment:31
<p>
By the way, I notice a lot of methods that have <a class="missing wiki">AttributeErrors?</a>. For example, <code>q.trailing_coefficient()</code> appears in the tab completion, but gives an error on invocation. Is filling out these methods that you've inherited (do "q." and then press the tab key to see the methods available/inherited) a planned feature for the future?
</p>
TicketnilesThu, 02 Sep 2010 20:58:52 GMT
https://trac.sagemath.org/ticket/9650#comment:32
https://trac.sagemath.org/ticket/9650#comment:32
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:30" title="Comment 30">jason</a>:
</p>
<blockquote class="citation">
<p>
I think it's fine to let the explicit conversion SR(q) work---by default, explicitly converting to SR just wraps the object as above, and I think is supported for every Sage object, whether or not it makes "sense". Do you see someplace that the conversion to an SR object is implicit? That would be a problem.
</p>
</blockquote>
<p>
I agree with Jason; I usually think of <code>SR</code> as "the nothing ring", or "the everything ring", depending on your point of view. The problem with <code>diff(q)</code> previously returning 0 has to do with <code>SR(q).vars()</code> being empty. That is, when converting to <code>SR</code>, the variables of <code>q</code> are not noticed. If you wanted to be overly fancy, you could try to implement something so that the variables of <code>q</code> are created and recognized when one does <code>SR(q)</code>, but this seems pretty pointless to me.
</p>
<p>
Also, Joris, I completely agree on your decision to raise an error for additional arguments to <code>diff</code>.
</p>
<p>
As for the method inheritance problem, I'm of two minds. This problem has come up for me (and probably others) too, and stems from wanting to inherit most of the functionality of another class, but not all of the methods. The technically correct thing to do is write a new master class, inherit from that, and rewrite the other class also inheriting from the new master class -- this will probably take a while, and has the potential to raise subtle bugs. The laziest thing to do is ignore the problem -- this will probably be fine for most users, and confusing for some. A middle road is to go through all of the inherited methods and make sense out of the ones you can. For those you can't, override the method to raise <code>NotImplementedError</code> or something similar, with a nice message about how this doesn't make sense for your class.
</p>
<p>
For this ticket, I'd be in support of raising <code>NotImplementedError</code>s for all of the broken inherited methods, and writing a new ticket to implement those that can be.
</p>
TicketjvkerschSat, 04 Sep 2010 21:28:03 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:33
https://trac.sagemath.org/ticket/9650#comment:33
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Thanks for elucidating the role of <code>SR</code>. I don't think there is any implicit coercion anywhere of forms into <code>SR</code>, so this probably won't give any problems.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:32" title="Comment 32">niles</a>:
</p>
<blockquote class="citation">
<p>
For this ticket, I'd be in support of raising <code>NotImplementedError</code>s for all of the broken inherited methods, and writing a new ticket to implement those that can be.
</p>
</blockquote>
<p>
OK, I've updated the patch so that the methods which are not applicable raise <code>NotImplementedError</code>. I don't know if I've done this properly though, since including all the doctests this amounts to quite a bit of additional code.
</p>
TicketjasonSat, 04 Sep 2010 21:51:56 GMT
https://trac.sagemath.org/ticket/9650#comment:34
https://trac.sagemath.org/ticket/9650#comment:34
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:33" title="Comment 33">jvkersch</a>:
</p>
<blockquote class="citation">
<p>
Thanks for elucidating the role of <code>SR</code>. I don't think there is any implicit coercion anywhere of forms into <code>SR</code>, so this probably won't give any problems.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:32" title="Comment 32">niles</a>:
</p>
<blockquote class="citation">
<p>
For this ticket, I'd be in support of raising <code>NotImplementedError</code>s for all of the broken inherited methods, and writing a new ticket to implement those that can be.
</p>
</blockquote>
<p>
OK, I've updated the patch so that the methods which are not applicable raise <code>NotImplementedError</code>. I don't know if I've done this properly though, since including all the doctests this amounts to quite a bit of additional code.
</p>
</blockquote>
<p>
I think you've gone above and beyond the requirement with this last one. Thanks! Again, I think the code looks good. If someone else wants to give a final positive review on the mathematical foundation, let's mark this positive review and get it in.
</p>
TicketjasonSat, 04 Sep 2010 21:52:07 GMTwork_issues deleted
https://trac.sagemath.org/ticket/9650#comment:35
https://trac.sagemath.org/ticket/9650#comment:35
<ul>
<li><strong>work_issues</strong>
<em>diff()</em> deleted
</li>
</ul>
TicketmhamptonSun, 05 Sep 2010 03:13:49 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:36
https://trac.sagemath.org/ticket/9650#comment:36
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Jason, I agree, I think this is in good enough shape for inclusion. I think its important to get this sort of new functionality in as soon as possible as long as the design looks reasonable, which it does.
</p>
<p>
I think this is very exciting, its the sort of functionality that Sage is meant for, and I'm very happy to see this addition.
</p>
TicketmpatelSun, 05 Sep 2010 03:57:55 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:37
https://trac.sagemath.org/ticket/9650#comment:37
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Could someone please update <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9650/trac9650_differential_forms_v2.patch" title="Attachment 'trac9650_differential_forms_v2.patch' in Ticket #9650">trac9650_differential_forms_v2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9650/trac9650_differential_forms_v2.patch" title="Download"></a> with a more descriptive commit string that includes the ticket number? For example: <code>#9650: Add support for differential forms</code>.
</p>
TicketjvkerschSun, 05 Sep 2010 07:52:07 GMTattachment set
https://trac.sagemath.org/ticket/9650
https://trac.sagemath.org/ticket/9650
<ul>
<li><strong>attachment</strong>
set to <em>trac9650_differential_forms_v2.patch</em>
</li>
</ul>
TicketjvkerschSun, 05 Sep 2010 07:53:00 GMT
https://trac.sagemath.org/ticket/9650#comment:38
https://trac.sagemath.org/ticket/9650#comment:38
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:37" title="Comment 37">mpatel</a>:
</p>
<blockquote class="citation">
<p>
Could someone please update <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9650/trac9650_differential_forms_v2.patch" title="Attachment 'trac9650_differential_forms_v2.patch' in Ticket #9650">trac9650_differential_forms_v2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9650/trac9650_differential_forms_v2.patch" title="Download"></a> with a more descriptive commit string that includes the ticket number? For example: <code>#9650: Add support for differential forms</code>.
</p>
</blockquote>
<p>
Done -- thanks for pointing that out.
</p>
TicketjvkerschSun, 05 Sep 2010 07:59:33 GMT
https://trac.sagemath.org/ticket/9650#comment:39
https://trac.sagemath.org/ticket/9650#comment:39
<p>
Hi Jason and Marshall,
</p>
<p>
Thanks a lot for the final positive review and thanks all for the many comments along the way -- I can honestly say that I've learned more about Sage (and even Python) in the last few weeks than ever before! Hopefully I can keep contributing in the future.
</p>
<p>
Best wishes,
Joris
</p>
TicketjvkerschSun, 05 Sep 2010 07:59:49 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:40
https://trac.sagemath.org/ticket/9650#comment:40
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketmpatelSun, 05 Sep 2010 08:02:25 GMTstatus changed
https://trac.sagemath.org/ticket/9650#comment:41
https://trac.sagemath.org/ticket/9650#comment:41
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketnilesSun, 05 Sep 2010 17:18:49 GMTattachment set
https://trac.sagemath.org/ticket/9650
https://trac.sagemath.org/ticket/9650
<ul>
<li><strong>attachment</strong>
set to <em>trac9650_differential_forms_v2-reviewer.patch</em>
</li>
</ul>
<p>
corrected formatting for <a class="missing wiki">NotImplemented?</a> methods
</p>
TicketnilesSun, 05 Sep 2010 17:26:05 GMT
https://trac.sagemath.org/ticket/9650#comment:42
https://trac.sagemath.org/ticket/9650#comment:42
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9650#comment:33" title="Comment 33">jvkersch</a>:
</p>
<blockquote class="citation">
<p>
OK, I've updated the patch so that the methods which are not applicable raise <code>NotImplementedError</code>. I don't know if I've done this properly though, since including all the doctests this amounts to quite a bit of additional code.
</p>
</blockquote>
<p>
This looks great, and I agree with others on getting it included into sage soon. There were some formatting errors in the new docstrings (need newline between <code>EXAMPLES::</code> and indented code block), and since someone else gave the positive review I just uploaded a corrected version (apply only <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9650/trac9650_differential_forms_v2-reviewer.patch" title="Attachment 'trac9650_differential_forms_v2-reviewer.patch' in Ticket #9650">trac9650_differential_forms_v2-reviewer.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9650/trac9650_differential_forms_v2-reviewer.patch" title="Download"></a>).
</p>
TicketjvkerschTue, 07 Sep 2010 09:33:23 GMT
https://trac.sagemath.org/ticket/9650#comment:43
https://trac.sagemath.org/ticket/9650#comment:43
<p>
Hi Niles,
</p>
<p>
Sorry for the formatting error and thanks for catching it so soon. I will go over everything again soon, and if I find any more inconsistencies which crept in during my last round of implementations, I will post a new patch.
</p>
TicketmpatelWed, 15 Sep 2010 10:39:29 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/9650#comment:44
https://trac.sagemath.org/ticket/9650#comment:44
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.6.alpha1</em>
</li>
</ul>
Ticket