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

convert magma power series rings #38794

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

fchapoton
Copy link
Contributor

this is trying to allow the conversion of magma's series rings

also refactors the conversion of polynomial rings

This needs to be tested, if you have magma.

📝 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 and checked the documentation preview.

Copy link

github-actions bot commented Oct 9, 2024

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

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

LGTM.

@fchapoton
Copy link
Contributor Author

Thanks Travis. Were you able to test that it works ? Sadly, I have no machine running both sage and magma.

@tscrim
Copy link
Collaborator

tscrim commented Oct 10, 2024

No, I don't have access to magma now. I just looked at the changes as I thought you had tested it. I will revert back to needs review then.

@fchapoton
Copy link
Contributor Author

yes, that's better indeed. Sorry

@fchapoton
Copy link
Contributor Author

It seems that NumberOfNames is only defined for polynomial rings...

@fchapoton
Copy link
Contributor Author

The interface seems to be broken for Galois groups.

Also the changes here seem to trigger an issue in src/sage/rings/number_field/number_field_element.pyx

@fchapoton
Copy link
Contributor Author

This is getting closer to be ready.

@fchapoton
Copy link
Contributor Author

this is ready for review. Testing all sage with with ticket and --optional=sage,magma gives only a few failures, some related to Galois groups (that needs to be fixed but not here) and other that are most probably unrelated to the changes made here

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Let's also get this in too.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 30, 2025
    
this is trying to allow the conversion of magma's series rings

also refactors the conversion of polynomial rings

This needs to be tested, if you have magma.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] 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 and checked the documentation
preview.
    
URL: sagemath#38794
Reported by: Frédéric Chapoton
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 31, 2025
    
this is trying to allow the conversion of magma's series rings

also refactors the conversion of polynomial rings

This needs to be tested, if you have magma.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] 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 and checked the documentation
preview.
    
URL: sagemath#38794
Reported by: Frédéric Chapoton
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 3, 2025
    
this is trying to allow the conversion of magma's series rings

also refactors the conversion of polynomial rings

This needs to be tested, if you have magma.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] 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 and checked the documentation
preview.
    
URL: sagemath#38794
Reported by: Frédéric Chapoton
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 4, 2025
    
this is trying to allow the conversion of magma's series rings

also refactors the conversion of polynomial rings

This needs to be tested, if you have magma.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] 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 and checked the documentation
preview.
    
URL: sagemath#38794
Reported by: Frédéric Chapoton
Reviewer(s): Travis Scrimshaw
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.

2 participants