Skip to content

get rid of CommutativeRing in p-adics #40160

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 1, 2025
Merged

get rid of CommutativeRing in p-adics #40160

merged 4 commits into from
Jun 1, 2025

Conversation

fchapoton
Copy link
Contributor

using Parent instead of the auld class CommutativeRing there

📝 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.

tscrim
tscrim approved these changes May 26, 2025
View reviewed changes
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.

@r-mb
Copy link
Contributor

r-mb commented May 26, 2025

Sorry, this is my fault for using CommutativeRing for Witt vectors. I followed the tutorial, I guess it's outdated?

You use base=coefficient_ring, maybe I'm confused because I did not know about the Parent stuff, but isn't the base used in this context to say that the Witt vector ring is a commutative coefficient_ring-algebra? If yes, the ring $W_n(R)$ is almost never a $R$-algebra. This is why I used self as the base.
Sorry if I understand this wrong.

@fchapoton
Copy link
Contributor Author

right, so the base ring should be fixed, todo

@fchapoton
Copy link
Contributor Author

@r-mb : what would be the best base ring ? ZZ or self ?

@r-mb
Copy link
Contributor

r-mb commented May 26, 2025

Both would be fine. I have chosen self because that's how it was done in some other part of the codebase. But ZZ should work fine because the code to convert an integer to a Witt vector is here. I don't know the technical implications of the choice so I don't know what's best.

r-mb
r-mb suggested changes May 27, 2025
View reviewed changes
@fchapoton
Copy link
Contributor Author

merci, voilà

r-mb
r-mb reviewed May 27, 2025
View reviewed changes
@fchapoton
Copy link
Contributor Author

ok, ok

@r-mb
Copy link
Contributor

r-mb commented May 27, 2025

(assuming all test pass as they do on my device)

vbraun pushed a commit to vbraun/sage that referenced this pull request May 28, 2025
8d3f32d 
sagemathgh-40160: get rid of CommutativeRing in p-adics
    
using `Parent` instead of the auld class `CommutativeRing` there

### 📝 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#40160
Reported by: Frédéric Chapoton
Reviewer(s): Rubén Muñoz--Bertrand, Travis Scrimshaw
@vbraun vbraun merged commit dcadc84 into sagemath:develop Jun 1, 2025
12 of 22 checks passed
@fchapoton fchapoton deleted the no_comm_ring_in_padics branch June 2, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tscrim tscrim tscrim approved these changes

@r-mb r-mb r-mb approved these changes

Assignees
No one assigned
Labels
c: padics s: positive review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fchapoton @r-mb @tscrim @vbraun