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

Decentralize sage.rings.numbers_abc #32641

Closed
mkoeppe opened this issue Oct 6, 2021 · 12 comments
Closed

Decentralize sage.rings.numbers_abc #32641

mkoeppe opened this issue Oct 6, 2021 · 12 comments
Labels
p: major / 3 t: enhancement t: refactoring
Milestone
sage-9.5

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 6, 2021

For the modularization, it's better to do the numbers ABC registration in the individual implementation modules.

We keep the module sage.rings.numbers_abc just for the doctests.

CC: @tscrim

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 8024209

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32641
@mkoeppe mkoeppe added this to the sage-9.5 milestone Oct 6, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 6, 2021

Branch: u/mkoeppe/decentralize_sage_rings_numbers_abc

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 23, 2021

Commit: 4296c63

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 23, 2021

New commits:

4296c63sage.rings.{integer,rational}: Copy numbers abc registration here from sage.rings.numbers_abc

@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 23, 2021

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 23, 2021

Changed commit from 4296c63 to 8024209

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 23, 2021

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1dc709csage.rings.{integer,rational}: Copy numbers abc registration here from sage.rings.numbers_abc
8024209sage.rings.numbers_abc: Move remaining numbers abc registrations to the individual implementation modules

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 26, 2021

comment:6

Patchbot is green except for the unrelated failure in src/sage/rings/integer.pyx.

@tscrim
Copy link
Collaborator

tscrim commented Oct 26, 2021

comment:7

IMO, this is a good idea anyways as it has better code localization.

@tscrim
Copy link
Collaborator

tscrim commented Oct 26, 2021

Reviewer: Travis Scrimshaw

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 26, 2021

comment:8

Thanks!

@vbraun
Copy link
Member

vbraun commented Oct 31, 2021

Changed branch from u/mkoeppe/decentralize_sage_rings_numbers_abc to 8024209

@vbraun vbraun closed this as completed in 2b36f5d Oct 31, 2021
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
@mkoeppe
Merge sagemath#32641
6c784c3
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
@mkoeppe
Merge sagemath#32641
8ccaa46
mkoeppe added a commit to mkoeppe/sage that referenced this issue Sep 21, 2023
@mkoeppe
src/sage/rings/numbers_abc.py: Remove function deprecated in sagemath…
0f9219c 
…#32641 (2021)
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 23, 2023
sagemathgh-36307: sage.rings: Remove code deprecated in sagemath#23204
9ea5127 
, sagemath#24483, sagemath#24371, sagemath#24511, sagemath#25848, sagemath#26105, sagemath#28481, sagemath#29010, sagemath#29412, sagemath#30332, sagemath#30372, sagemath#31345, sagemath#32375, sagemath#32606, sagemath#32610, sagemath#32612, sagemath#32641, sagemath#32660, sagemath#32750, sagemath#32869, sagemath#33602

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36307
Reported by: Matthias Köppe
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
p: major / 3 t: enhancement t: refactoring
Projects
None yet
Milestone
sage-9.5
Development

No branches or pull requests
3 participants
@vbraun @tscrim @mkoeppe