Opened 11 years ago
Closed 11 years ago
#9650 closed enhancement (fixed)
Adding support for differential forms
Reported by: | jvkersch | Owned by: | burcin |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6 |
Component: | symbolics | Keywords: | forms, functions, symbolics |
Cc: | mhampton, jason, jdemeyer, mpatel, novoselt, niles | Merged in: | sage-4.6.alpha1 |
Authors: | Joris Vankerschaver | Reviewers: | Niles Johnson |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Implements support for differential forms to Sage. After applying the patch, three new classes are added to sage:
CoordinatePatch
-- an open subset of Rn on which differential forms can be definedDifferentialForms
-- a differential forms parentDifferentialForm
-- a differential forms class.
Please see the documentation in the DifferentialForm
class for more information about syntax, available methods, etc.
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.
See discussion at
Attachments (2)
Change History (46)
comment:1 Changed 11 years ago by
- Cc jason added
comment:2 Changed 11 years ago by
- Status changed from new to needs_work
comment:3 Changed 11 years ago by
Also, if you feel like the patch is to the point that it needs review, please change the status below to "needs review".
comment:4 Changed 11 years ago by
- Milestone set to sage-4.5.2
comment:5 Changed 11 years ago by
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!
Some quick comments and questions:
- 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).
- I'm very interesting in seeing how the authors of that mathematica package represented differential forms -- thanks for the link!
- Once I've taken a good look at the mathematica package, I'll set the patch to "needs review".
comment:6 Changed 11 years ago by
- Cc jdemeyer added
comment:7 Changed 11 years ago by
- Status changed from needs_work to needs_review
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.
Some things to keep in mind: right now, the way to create a differential form is as follows: first you create a ``CoordinatePatch?`` on which forms can be defined, then you create a ``DifferentialForms?`` parent and then you can create forms. Explicitly, this looks like
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)
Let me know if this construction is confusing or can be simplified in any way.
comment:8 Changed 11 years ago by
Oops: inside of triple-braces, things are quoted literally:
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)
comment:9 Changed 11 years ago by
- Cc novoselt added
comment:10 Changed 11 years ago by
comment:11 Changed 11 years ago by
- Cc mpatel added
comment:12 Changed 11 years ago by
- Cc mhampton added
comment:13 Changed 11 years ago by
To placate ./sage -b
, which now gives
package init file 'sage/tensor/__init__.py' not found (or not a regular file)
messages, could you add an empty sage/tensor/__init__.py
.
Also, if you'd like to document your new modules in the reference manual, you can add tensor
to
SAGE_ROOT/devel/sage/doc/en/reference/index.rst
and create tensor.rst
, patterned after graphs.rst
(say), in the same directory. To rebuild the manual, run
$ cd SAGE_ROOT $ ./sage -b $ ./sage -docbuild reference html -j
Sphinx should print warnings about any docstring formatting problems. The results will be in
SAGE_ROOT/devel/sage/doc/output/html/en/reference/
comment:14 follow-up: ↓ 15 Changed 11 years ago by
- Cc niles added
- Reviewers set to Niles Johnson
- Status changed from needs_review to needs_work
- Work issues set to __init__.py, documentation warnings, gens()
Hi Joris,
I saw your e-mail to sage-devel
, 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).
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
- add empty file init.py (as mentioned above; this is required for doctesting and to build documentation)
- add author to coordinate_patch.py
- I believe it is usual for gens() to return a tuple (rather than a list); e.g.
sage: PolynomialRing(RR,3,'x,y,z').gens() (x, y, z)
make ptestlong
passes all tests!
- Following the above suggestion, I added 'tensor' to
index.rst
and the following to a new filetensor.rst
. There are a few warnings when building the documentation that need to be fixed, but mostly it looks good.Differential Forms ============ .. toctree:: :maxdepth: 2 sage/tensor/coordinate_patch sage/tensor/differential_forms sage/tensor/differential_form_element
- Is there a reason you call the directory
tensor
instead of e.g.diff_forms
or evendifferential_forms
? I'm not an expert on differential forms, so I defer to your judgment, but the latter seems more intuitive to me.
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 11 years ago by
the first point is about __init__.py
; sorry for the misformatting!
comment:16 in reply to: ↑ 15 Changed 11 years ago by
oh, and doctest coverage is at 100% for each of the 3 files!
comment:17 follow-up: ↓ 18 Changed 11 years ago by
Hi Niles and Mitesh,
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.
I implemented the changes you suggested (adding to the reference manual, adding authors, making gens()
return a tuple), but I have a few questions/comments that are more of a technical nature, revealing simultaneously my mercurial ineptitude:
- I made a mess of the upload section -- could someone with admin privileges delete all but the most recent attachment?
- Try as I might, I could not produce a patch which creates the file
__init__.py
. I added that file to my sage tree as per the documentation, instructed hg to add it, confirmed withhg status
that it was listed as added, but when I look athg diff
that file is nowhere to be found. The other files are created/modified as expected. The patch also does not create__init__.py
, as you experienced. Are these init files somehow special as far as hg is concerned?
- I called the package
tensor
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.
comment:18 in reply to: ↑ 17 ; follow-up: ↓ 19 Changed 11 years ago by
Replying to jvkersch:
Hi Niles and Mitesh,
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.
I implemented the changes you suggested (adding to the reference manual, adding authors, making
gens()
return a tuple), but I have a few questions/comments that are more of a technical nature, revealing simultaneously my mercurial ineptitude:
- I made a mess of the upload section -- could someone with admin privileges delete all but the most recent attachment?
Done.
- Try as I might, I could not produce a patch which creates the file
__init__.py
. I added that file to my sage tree as per the documentation, instructed hg to add it, confirmed withhg status
that it was listed as added, but when I look athg diff
that file is nowhere to be found. The other files are created/modified as expected. The patch also does not create__init__.py
, as you experienced. Are these init files somehow special as far as hg is concerned?
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:
# This comment is here so the file is non-empty (so Mercurial will check it in).
or something like that.
- I called the package
tensor
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.
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.
comment:19 in reply to: ↑ 18 Changed 11 years ago by
- Status changed from needs_work to needs_review
Replying to jason:
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: (...)
Thanks a lot for pointing that out -- this is really helpful!
I've updated the patch with the __init__.py
, verified that it passes all doctests, and added those files to the reference manual section. So I'm changing the status back to needs_review
.
comment:20 Changed 11 years ago by
It looks like the top of coordinate_patch.py is repeated around line 232. Was that intentional?
comment:21 Changed 11 years ago by
In fact, it looks like the entire content of coordinate_patch.py is duplicated, starting around line 232 of that file.
comment:22 Changed 11 years ago by
- Status changed from needs_review to needs_work
- Work issues changed from __init__.py, documentation warnings, gens() to documentation formatting, duplicates in coordinate_patch.py
Hi Joris,
Things are looking good, but the documentation produces some warnings related to formatting errors--please fix this in addition to deleting the duplicates from coordinate_patch.py
. There are also some formatting problems in the documentation which do not produce warnings, so please check the html documentation carefully.
Also, do you think the documentation files could have better names? I find it funny that the documentation for "DifferentialForm
" is titled "Differential forms class" and the documentation for "DifferentialForms
" is titled something else. (But again I'll defer to you on this :)
comment:23 Changed 11 years ago by
Hi Jason and Niles,
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:
- 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...
- I agree on the names of the documentation files. In the current patch, the documentation now reads: "Algebra of differential forms" for
differential_forms.py
and "Elements of the algebra of differential forms" fordifferential_form_element.py
. 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!
Thanks for all your time on the review!
comment:24 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:25 Changed 11 years ago by
Joris, could you add yourself to the account name-real name map, when it's convenient?
comment:26 Changed 11 years ago by
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?).
comment:27 follow-up: ↓ 28 Changed 11 years ago by
This looks good to me too, although I'm not sure if this is desired behavior:
U = CoordinatePatch((x,y)) F2 = DifferentialForms(U) q = DifferentialForm(F2,1) q[0] = -y q[1] = x diff(q,y)
gives
0
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.
comment:28 in reply to: ↑ 27 Changed 11 years ago by
- Status changed from needs_review to needs_work
- Work issues changed from documentation formatting, duplicates in coordinate_patch.py to diff()
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)
Replying to mhampton:
This looks good to me too, although I'm not sure if this is desired behavior:
U = CoordinatePatch((x,y)) F2 = DifferentialForms(U) q = DifferentialForm(F2,1) q[0] = -y q[1] = x diff(q,y)
gives
0
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.
This problem is caused because diff()
first tries to call q.differentiate()
and, failing that, coerces q
to the symbolic ring and differentiates it there. Adding a differentiate()
method which simply calls q.diff()
would solve this problem. Depending on what you think is best, you could silently ignore additional arguments to diff
, or you could throw an error if additional arguments are present.
comment:29 Changed 11 years ago by
Hi Marshall and Niles,
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 derivative
member function which calls diff
. 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.
More generally, Marshall's issue uncovers something strange having to do with coercion into the symbolic ring:
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
I.e, if I coerce q
into the symbolic ring, it still looks like a differential form but its parent is SR
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?
Let me know what you guys think -- for now I'll leave this as needs_work.
comment:30 follow-up: ↓ 32 Changed 11 years ago by
Think of SR as a wrapper around any python object. For example, continuing from your example above
sage: p=SR(q) sage: p.pyobject().parent() Algebra of differential forms in the variables x, y
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.
comment:31 Changed 11 years ago by
By the way, I notice a lot of methods that have AttributeErrors?. For example, q.trailing_coefficient()
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?
comment:32 in reply to: ↑ 30 ; follow-up: ↓ 33 Changed 11 years ago by
Replying to jason:
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.
I agree with Jason; I usually think of SR
as "the nothing ring", or "the everything ring", depending on your point of view. The problem with diff(q)
previously returning 0 has to do with SR(q).vars()
being empty. That is, when converting to SR
, the variables of q
are not noticed. If you wanted to be overly fancy, you could try to implement something so that the variables of q
are created and recognized when one does SR(q)
, but this seems pretty pointless to me.
Also, Joris, I completely agree on your decision to raise an error for additional arguments to diff
.
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 NotImplementedError
or something similar, with a nice message about how this doesn't make sense for your class.
For this ticket, I'd be in support of raising NotImplementedError
s for all of the broken inherited methods, and writing a new ticket to implement those that can be.
comment:33 in reply to: ↑ 32 ; follow-ups: ↓ 34 ↓ 42 Changed 11 years ago by
- Status changed from needs_work to needs_review
Thanks for elucidating the role of SR
. I don't think there is any implicit coercion anywhere of forms into SR
, so this probably won't give any problems.
Replying to niles:
For this ticket, I'd be in support of raising
NotImplementedError
s for all of the broken inherited methods, and writing a new ticket to implement those that can be.
OK, I've updated the patch so that the methods which are not applicable raise NotImplementedError
. 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.
comment:34 in reply to: ↑ 33 Changed 11 years ago by
Replying to jvkersch:
Thanks for elucidating the role of
SR
. I don't think there is any implicit coercion anywhere of forms intoSR
, so this probably won't give any problems.Replying to niles:
For this ticket, I'd be in support of raising
NotImplementedError
s for all of the broken inherited methods, and writing a new ticket to implement those that can be.OK, I've updated the patch so that the methods which are not applicable raise
NotImplementedError
. 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.
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.
comment:35 Changed 11 years ago by
- Work issues diff() deleted
comment:36 Changed 11 years ago by
- Status changed from needs_review to positive_review
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.
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.
comment:37 follow-up: ↓ 38 Changed 11 years ago by
- Status changed from positive_review to needs_work
Could someone please update trac9650_differential_forms_v2.patch with a more descriptive commit string that includes the ticket number? For example: #9650: Add support for differential forms
.
Changed 11 years ago by
comment:38 in reply to: ↑ 37 Changed 11 years ago by
Replying to mpatel:
Could someone please update trac9650_differential_forms_v2.patch with a more descriptive commit string that includes the ticket number? For example:
#9650: Add support for differential forms
.
Done -- thanks for pointing that out.
comment:39 Changed 11 years ago by
Hi Jason and Marshall,
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.
Best wishes, Joris
comment:40 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:41 Changed 11 years ago by
- Status changed from needs_review to positive_review
comment:42 in reply to: ↑ 33 Changed 11 years ago by
Replying to jvkersch:
OK, I've updated the patch so that the methods which are not applicable raise
NotImplementedError
. 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.
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 EXAMPLES::
and indented code block), and since someone else gave the positive review I just uploaded a corrected version (apply only trac9650_differential_forms_v2-reviewer.patch).
comment:43 Changed 11 years ago by
Hi Niles,
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.
comment:44 Changed 11 years ago by
- Merged in set to sage-4.6.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
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:
all([is_SymbolicVariable(c) for c in coordinates])
should not construct a list, so that short-circuiting can occur:all(is_SymbolicVariable(c) for c in coordinates)
None
should be done with is (it's a lot faster that way):metric is not None
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?