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

Implement the hypercenter and upper central series for finite dimensional Lie algebras #37693

Merged
merged 4 commits into from May 2, 2024

Conversation

tscrim
Copy link
Collaborator

@tscrim tscrim commented Mar 29, 2024

We implement the upper central series and hypercenter of a finite dimensional Lie algebra.

As part of the testing, we found improvements could be made to the generic example (needed so the example would work with these tests): We moved the reduce() method up to the category. Implemented methods expected for the subalgebras. Better input verification. Elements are now hashable. We had to work around the fact that lift() is serving multiple roles as to the UEA and to the ambient space for a Lie subalgebra. This functionality needs to be divided (I take full responsibility for this bad design) by renaming lift to something for the UEA map, but this is a significant change that should be done on a followup PR.

We also fixed some bugs and improved efficiency with changing vectors into Lie algebra elements.

📝 Checklist

  • The title is concise and informative.
  • 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

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 8, 2024

I've added all of the suggested whitespace.

Comment on lines 221 to 225
try:
amb_names = dict(zip(sorted_indices, ambient.variable_names()))
names = [amb_names[i] for i in index_set]
except ValueError: # ambient has not assigned variable names
pass
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to rewrite it as try... except... else, which makes it clearer which of the lines is expected to raise the exception

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I instead just examined the _names attribute and avoided using try-except to control programming flow.

Copy link
Member

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

Copy link

github-actions bot commented Apr 9, 2024

Documentation preview for this PR (built with commit 4fa759a; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 28, 2024
…for finite dimensional Lie algebras

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

We implement the upper central series and hypercenter of a finite
dimensional Lie algebra.

As part of the testing, we found improvements could be made to the
generic example (needed so the example would work with these tests): We
moved the `reduce()` method up to the category. Implemented methods
expected for the subalgebras. Better input verification. Elements are
now hashable. We had to work around the fact that `lift()` is serving
multiple roles as to the UEA and to the ambient space for a Lie
subalgebra. This functionality needs to be divided (I take full
responsibility for this bad design) by renaming `lift` to something for
the UEA map, but this is a significant change that should be done on a
followup PR.

We also fixed some bugs and improved efficiency with changing vectors
into Lie algebra elements.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37693
Reported by: Travis Scrimshaw
Reviewer(s): Matthias Köppe, Travis Scrimshaw
@vbraun vbraun merged commit e4e3325 into sagemath:develop May 2, 2024
12 checks passed
@tscrim tscrim deleted the lie_algebras/hypercenter branch May 2, 2024 23:00
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 2, 2024
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.

None yet

3 participants