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.

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

make a category of Dedekind domains, remove code from ring.pyx #37234

Merged
merged 5 commits into from
Feb 13, 2024
Merged

make a category of Dedekind domains, remove code from ring.pyx #37234

merged 5 commits into from
Feb 13, 2024

Conversation

fchapoton
Copy link
Contributor

This is creating a DedekindDomains category and moving the associated code from ring.pyx to the category.

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

@fchapoton
Copy link
Contributor Author

maybe one should make ZZ belong to this category ?

tscrim
tscrim requested changes Feb 5, 2024
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.

One tiny thing (it should be fixed more broadly though). However, I think it is a good thing to refine the category of ZZ and remove duplicate methods (these should never be a critical part of any algorithm IMO, so downgrading Cython -> Python shouldn't have any real effect...I hope).

@fchapoton
Copy link
Contributor Author

It seems that ZZ prefers to inherit krull_dimension from rings/ring.pyx. Same for is_integrally_closed. Not sure how to fix that.

@fchapoton
Copy link
Contributor Author

a real can of worms. One would like to make ZZ based on Parent, but this is not simple at all.

I would rather undo the removal of these two methods in ZZ for the moment, if you agree.

@fchapoton fchapoton force-pushed the dedekind_domain_category branch from 37c4e9f to c85a268 Compare February 5, 2024 11:26
@tscrim
Copy link
Collaborator

tscrim commented Feb 5, 2024

Ah, right, categories have the lowest precedence. Yes, we can revert removing them. It was just trying to avoid some redundancies.

@fchapoton fchapoton force-pushed the dedekind_domain_category branch from c24281c to 8469f2c Compare February 5, 2024 14:30
tscrim
tscrim requested changes Feb 6, 2024
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.

Thank you.

One last thing, but unfortunately it is somewhat annoying. We have an import of DedekindDomain in rings/all.py, which makes its way into the global namespace. So we need to properly formally deprecate the class. sigh

@fchapoton
Copy link
Contributor Author

fchapoton commented Feb 6, 2024 •
edited
Loading

ok, right. Annoying it was.

Because this was informally deprecated for a long time, I have just put a minimal replacement in place.

I have also (in the same commit) replaced all general imports of rings.ring by more specific imports. This can be undone if this causes problems.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2024

Documentation preview for this PR (built with commit 7b3bb5b; changes) is ready! 🎉

tscrim
tscrim approved these changes Feb 6, 2024
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.

Thank you. Once the bot shows tests pass, you can set a positive review.

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 7, 2024
d8a8477 
sagemathgh-37234: make a category of Dedekind domains, remove code from ring.pyx
    
This is creating a `DedekindDomains` category and moving the associated
code from `ring.pyx` to the category.

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [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.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#37234
Reported by: Frédéric Chapoton
Reviewer(s): Travis Scrimshaw
@vbraun vbraun merged commit b33cb75 into sagemath:develop Feb 13, 2024
17 of 22 checks passed
@fchapoton fchapoton deleted the dedekind_domain_category branch February 14, 2024 07:23
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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

Assignees
No one assigned
Labels
t: refactoring
Projects
None yet
Milestone
sage-10.3
Development

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