-
-
Notifications
You must be signed in to change notification settings - Fork 482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add symplectic structures #30362
Comments
comment:1
In my humble opinion, I think the very first step should be to establish a Poisson manifold. This is much more general, i.e. symplectic structures / manifolds are special cases of them. Since none of these are implemented yet, it is much easier to start with the more general setup.
I am still busy with other things, but as soon as I have some free time, I could go through your code. However, it would be good to at least remove unneccessary methods (like e.g. I noticed that you already added typing. I think, this belongs to another ticket, namely #29775, and should be discussed there before we apply it to new things. There is still this issue with pyflakes and it seems there are still different opinions.
The use of |
comment:2
I swiftly overviewed your code. It's a nice addition! However, I have some comments:
PseudoRiemannianMetric._del_derived(self)
+ self._del_inverse()
def _del_inverse(self):
"""
- def __init__(self, n, name, field, structure, base_manifold=None,
+ def __init__(self, n, name, field='real', structure=RealDifferentialStructure(), base_manifold=None,
diff_degree=infinity, latex_name=None, start_index=0,
category=None, unique_tag=None):
|
comment:4
Symplectic structures are certainly a nice addition to Sage! Thank you for contributing to this. I gave a look to the code, but I have to say that in the current state, it is hardly readable for me, because the docstrings are simply copied from other files, without any cleaning nor adaptation of the doctests to the new features introduced here. Can I suggest that you move forward and provide a first version with some minimal documentation and doctests, cleaning out what does not pertain to this ticket. Replying to @mjungmath:
Besides, I agree with Michael's remarks in comment:2. |
comment:5
Replying to @egourgoulhon:
+1 |
comment:6
Thanks for the feedback. I'll cleanup and improve the code accordingly. I might need a couple of weeks for this however as I'm currently quite busy. |
comment:7
Thanks for the initial reviews! Replying to @mjungmath:
Agreed! Not sure why I added it in the first place, but I've now reverted this change.
The terminology sharp and flat (as well as musical isomorphisms) is standard in symplectic geometry, too. See for example Abraham Marsden Foundations of Mechanics.
I think, it's strange if the constructor requires more data than actually is required. Although that might be a matter of taste, but I find
Agreed! But I would like to leave the categorical questions to a follow-up ticket!
I'm working on it. |
This comment has been minimized.
This comment has been minimized.
comment:9
You have a page? I only find the terminology 'lowering' and 'raising'. |
comment:10
In the second edition, its definition 3.2.1 (p. 174, chapter about symplectic geometry). They don't write "sharp" or "flat" as words, but use the musical symbols. |
comment:11
Replying to @tobiasdiez:
I agree, but the reason for using the function |
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
|
comment:13
Ok, makes sense! I've continued working on it, introduced Poisson structures, wrote most of the documentation and introduced a few tests. It's not yet finished, but if you have a spare minute I would like to get feedback. A few questions:
How do I make sure that it derives from Moreover, do I have to copy-paste the documentation from the parent class, or what is the convention?
but what would be the version on a sphere (in such a way that the code works for M being a vector space and a sphere)?
Thanks! |
comment:14
Replying to @tobiasdiez:
You should construct the Poisson tensor from the module of vector fields on the manifold, not by a direct call to
I don't understand the question, sorry. Could you rephrase it?
The symbolic functions created by
where
The Hodge star is already in
I would say simply A question from my side: why is this ticket depending on #30901 and #30748 ? In the ticket description you write Please ignore the dependencies, these are just there so that I can work locally on my PC. This looks somewhat odd... Maybe this is related, but the ticket branch contains the following modified files:
Do they really pertain to this ticket? |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:16
Replying to @egourgoulhon:
Ok, thanks! That makes sense indeed and worked.
If I implement a method in say
That worked! Merci!
That worked as well!
Sorry for these (unnessary) changes. The only way I currently have to work on sage is with the virtual environment created in #30371. The compiled cython however only works correctly if the changes of #30901 and #30748 are included. Thus, until these packages are merged, I have to sadly include them as dependencies on all packages I develop...maybe you or Matthias have a better idea for a workaround. Now all tests are passing (not sure about the doctests), and the only thing left is to change the documentation a bit. New commits:
|
comment:17
Replying to @tobiasdiez:
Indeed, this is not the way to do this.
I would suggest to create a clean branch starting from the latest beta and to push that to the ticket only. Use |
Author: Tobias Diez |
This comment has been minimized.
This comment has been minimized.
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:166
Thanks for the review and the fix! |
comment:167
Looks good to me too. |
comment:168
Last question: do we really need to import everything in the test files? Otherwise, LGTM. |
comment:169
Replying to @mjungmath:
Sadly yes, at the moment, since there are a few cyclic imports that are only resolved using the |
comment:170
Then we should probably add a |
comment:171
Merge conflict |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:173
Merge conflict fixed. |
Changed branch from public/manifolds/symplectic to |
comment:176
Now that the branch has been merged in Sage 9.6.beta0 (unfortunately it did not make its way to Sage 9.5...), there remains a last thing to do: preparing some examples of use for Sage 9.6 release tour, on the same footing as what has been done for Sage 9.5: |
Changed commit from |
comment:177
Is a copy&paste from the doctests good enough? Should I paste it here then, or how does one get access to the wiki? |
comment:178
Replying to @tobiasdiez:
It would be a bit better to give a very quick summary and some simple example usage (which can by c/p from the doctests).
You can edit the wiki by logging in with your trac account info. I don't know if it works for gh authentication. |
comment:179
Replying to @egourgoulhon:
Ping |
comment:180
Another ping, pointing to the new release tour page, which is editable via a github login: |
comment:181
Sorry, I've been somewhat busy and forgot about this. Thanks for the reminder! I've now updated the release notes with a quick introduction. |
comment:182
Replying to @tobiasdiez:
Thanks! |
This ticket implements the basics of symplectic structures,
like Poisson brackets and Hamiltonian vector fields.
TODO (as follow-up tickets):
EuclideanSpace
to new class
VectorSpace
, and letSymplecticVectorSpace
derive from
VectorSpace
CC: @tscrim @nthiery @mjungmath @egourgoulhon @mkoeppe
Component: manifolds
Author: Tobias Diez
Branch:
b78d8a2
Reviewer: Eric Gourgoulhon, Michael Jung, Matthias Koeppe, Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/30362
The text was updated successfully, but these errors were encountered: