Skip to content
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 a few operations from linear symplectic geometry #35354

Merged
merged 17 commits into from
May 22, 2023

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Mar 26, 2023

📚 Description

  • Add trace of a (0,2)-tensor with respect to the symplectic form (or metric)
  • Add extension of the symplectic form to a bilinear form on p-forms
  • Fix hodge star for symplectic manifolds (was off by a minus sign for odd-degree forms)
  • Add option for hodge star to add an additional factor of (-1)^s with s being the number of negative eigenvalues of the metric, that is,
    \alpha \wedge \star \beta = (-1)^s g(\alpha, \beta) vol_g

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@egourgoulhon
Copy link
Member

egourgoulhon commented Mar 27, 2023

However, now the hodge star on pseudo-Riemannian manifolds is wrong. What is your convention there?

The convention for the Hodge star on a pseudo-Riemannian manifold is detailed here.
I guess it should not be changed in order not to break backward compatibility.

@egourgoulhon
Copy link
Member

BTW, I'm happy to see that symplectic manifolds are back!

@tobiasdiez
Copy link
Contributor Author

However, now the hodge star on pseudo-Riemannian manifolds is wrong. What is your convention there?

The convention for the Hodge star on a pseudo-Riemannian manifold is detailed here. I guess it should not be changed in order not to break backward compatibility.

Thanks! I only added the -1 factor because I got confused by some failing tests - but they were actually failing because of other reasons. As the (-1)^s convention is quite popular, I kept it behind a toggle.

Tests pass, so this is now ready for review.

Copy link
Member

@egourgoulhon egourgoulhon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difficulty in providing an example of use of using in FreeModuleTensor.trace points towards reimplementing the method trace in class TensorFieldParal, leaving FreeModuleTensor.trace without the argument using. This would also get rid of the modularity issue.

src/sage/tensor/modules/free_module_tensor.py Outdated Show resolved Hide resolved
src/sage/tensor/modules/free_module_tensor.py Outdated Show resolved Hide resolved
src/sage/tensor/modules/free_module_tensor.py Show resolved Hide resolved
tobiasdiez and others added 2 commits April 19, 2023 16:35
Co-authored-by: Eric Gourgoulhon <eric.gourgoulhon@obspm.fr>
Co-authored-by: Eric Gourgoulhon <eric.gourgoulhon@obspm.fr>
@tobiasdiez
Copy link
Contributor Author

The difficulty in providing an example of use of using in FreeModuleTensor.trace points towards reimplementing the method trace in class TensorFieldParal, leaving FreeModuleTensor.trace without the argument using. This would also get rid of the modularity issue.

Mhh, but doesn't this complicate things by having three implementations of taking the trace?

@egourgoulhon
Copy link
Member

The difficulty in providing an example of use of using in FreeModuleTensor.trace points towards reimplementing the method trace in class TensorFieldParal, leaving FreeModuleTensor.trace without the argument using. This would also get rid of the modularity issue.

Mhh, but doesn't this complicate things by having three implementations of taking the trace?

Code duplication can be avoided by having TensorFieldParal.trace call FreeModuleTensor.trace once the up operation is performed. The major drawback is somehow loosing the large bunch of documentation of FreeModuleTensor.trace, which is currently returned by t.trace? where t is a TensorFieldParal. The advice of Matthias would be helpful here.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 19, 2023

That we lose the documentation of a method from a superclass whenever we override it is a general problem, which seems to be worked around by a lot of copy-paste in the existing code base. It would be great to have a general solution for that. As doc processing for the terminal is under our control (in sage.misc.sagedoc), I think it's not too hard to come up with special markup for "include super's method documentation here". (In the HTML documentation, it could just be a hyperlink, I suppose.)

@egourgoulhon
Copy link
Member

Thank you Matthias for your feedback. It would indeed be nice to have such a feature for the documentation.

Here, the ultimate solution would be to implement the linear algebra versions of up and down, as pointed out by Tobias in #35354 (comment). But this is beyond the scope of this ticket.

@tobiasdiez
Copy link
Contributor Author

I've now added a test for the changes in free_module_tensor. This seems to be the easiest way out for now, since the other solution (copy & paste the whole docstring for trace from tensofield to tensorfield_paral) seems to be more of maintenance burden than providing additional value. Will implement the up/down methods for normal tensors in a follow-up PR (and maybe think about a general solution how operations from linear algebra can be lifted to tensor fields by applying them pointwise).

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: b439ea1

Copy link
Member

@egourgoulhon egourgoulhon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this!

@tobiasdiez
Copy link
Contributor Author

Thanks for the review, Eric and Matthias!

@vbraun vbraun merged commit e154bea into sagemath:develop May 22, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 22, 2023
@tobiasdiez tobiasdiez deleted the lin_symplectic branch May 23, 2023 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants