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

Refactor ring categories #37719

Merged
merged 24 commits into from
May 12, 2024
Merged

Conversation

fchapoton
Copy link
Contributor

@fchapoton fchapoton commented Apr 1, 2024

A somewhat large refactoring of some of the auld classes for rings, and related categories

  • introducing a new category of Noetherian rings
  • moving some methods in appropriate categories
  • fixing necessary details

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

Depends on #37778

@fchapoton
Copy link
Contributor Author

damn, there remains annoying failing doctests in src/sage/matrix !

@fchapoton
Copy link
Contributor Author

fchapoton commented Apr 3, 2024

One of the remaining bugs is now

sage: A = Zmod(1)
sage: B = A['x']
sage: B.one()
crash in coercion somehow

@tscrim
Copy link
Collaborator

tscrim commented Apr 4, 2024

Okay, I've figured it out. The change in the category from Algebras to Rings means that the base ring coercion is no longer provided by default. It's actually somewhat amazing that this doesn't end up going through an infinite loop, but that is how coerce_map_from works with its caching. If you add

        if P == base_ring:
            return self._coerce_map_from_base_ring()

to the beginning of PolynomialRing_general._coerce_map_from_(), then the coercion will work. It's a bit of a dirty workaround, but I don't see how to do it any better. The other option I considered is doing a second base_ring.is_zero() at the end of the __init__ and registering the coercion manually. Yet this felt even less clean, but I can't really say why though. (I am also surprised the multivariate case works; I don't understand how it does at this point.)

There are some other issues/inconsistencies with polynomials over $F_1$:

sage: B = A['x,y']
sage: B.gens()
(0, 0)
sage: B = A['x']
sage: B.gens()
(x,)

The univariate case is wrong IMO.

The change in multi_polynomial.pyx is for the polynomial ring with no generators, correct? Can we add a test for this?

I am not sure about the change for LocalGeneric to Parent from CommutativeRing because the latter class seems to have methods not provided by the category (e.g., localization).

@fchapoton
Copy link
Contributor Author

Thanks a lot, Travis ! I was worrying to be unable to debug that issue about Zmod(1)[x].

I propose to keep the other inconsistencies of polynomials over the zero ring for later.

I have added the suggested doctest in multi_polynomial.

For the change in local_generic, I do not remember why I was forced to do that. In any case, it contributes to the future possible removal of CommutativeRing, one day.

I have disabled a doctest in the testsuite of Laurent polynomials over Zmod(2) about gcd.

@fchapoton
Copy link
Contributor Author

no idea what the CI complains about. Something about "modularity", it seems......

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 5, 2024

no idea what the CI complains about

The failure is unrelated to this PR and tracked in #37734.

Copy link
Contributor

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

LGTM

@grhkm21
Copy link
Contributor

grhkm21 commented Apr 6, 2024

Setting to positive since Matthias approved, unless Travis disagrees.

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 comes back green, positive review.

Copy link

github-actions bot commented Apr 12, 2024

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

@fchapoton
Copy link
Contributor Author

As expected, the lights are not green. As I said, I have no idea if it is possible to declare a dependency in such a way that the CI will merge the branches before testing.

@mantepse
Copy link
Collaborator

As expected, the lights are not green. As I said, I have no idea if it is possible to declare a dependency in such a way that the CI will merge the branches before testing.

As far as I know, you have to merge the dependency into your branch.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2024

I've repaired the CI. It complains:

sage -t --random-seed=232623843441133993949100432847335930207 src/sage/matrix/matrix_space.py
**********************************************************************
File "src/sage/matrix/matrix_space.py", line 493, in sage.matrix.matrix_space.MatrixSpace
Failed example:
    MatrixSpace(ZZ, 10, 5).category()
Expected:
    Category of infinite enumerated finite dimensional modules with basis over
     (Dedekind domains and euclidean domains
      and infinite enumerated sets and metric spaces)
Got:
    Category of infinite enumerated finite dimensional modules with basis over (Dedekind domains and euclidean domains and noetherian rings and infinite enumerated sets and metric spaces)
**********************************************************************
File "src/sage/matrix/matrix_space.py", line 497, in sage.matrix.matrix_space.MatrixSpace
Failed example:
    MatrixSpace(ZZ, 10, 10).category()
Expected:
    Category of infinite enumerated finite dimensional algebras with basis over
     (Dedekind domains and euclidean domains
      and infinite enumerated sets and metric spaces)
Got:
    Category of infinite enumerated finite dimensional algebras with basis over (Dedekind domains and euclidean domains and noetherian rings and infinite enumerated sets and metric spaces)
**********************************************************************
1 item had failures:
   2 of  50 in sage.matrix.matrix_space.MatrixSpace
    [4[60](https://github.com/sagemath/sage/actions/runs/8948624007/job/24713068325?pr=37719#step:12:61) tests, 2 failures, 2.97 s]

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2024

You can fix the failure in "test-mod" like this:

diff --git a/pkgs/sagemath-categories/known-test-failures.json b/pkgs/sagemath-categories/known-test-failures.json
index ab27f65ca8d..f8303460c94 100644
--- a/pkgs/sagemath-categories/known-test-failures.json
+++ b/pkgs/sagemath-categories/known-test-failures.json
@@ -597,6 +597,10 @@
         "failed": true,
         "ntests": 126
     },
+    "sage.categories.noetherian_rings": {
+        "failed": true,
+        "ntests": 19
+    },
     "sage.categories.number_fields": {
         "failed": true,
         "ntests": 41

(see https://deploy-livedoc--sagemath.netlify.app/html/en/developer/doctesting#updating-baseline-files)

def extra_super_categories(self):
r"""
Let Sage knows that finite commutative rings
are Noetherian.
Copy link
Contributor

Choose a reason for hiding this comment

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

check whether you want to use "Noetherian" or "noetherian" throughout the text

fchapoton and others added 2 commits May 9, 2024 08:04
Co-authored-by: Matthias Köppe <mkoeppe@math.ucdavis.edu>
Copy link
Contributor

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

LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
    
A somewhat large refactoring of some of the auld classes for rings, and
related categories

- introducing a new category of Noetherian rings
- moving some methods in appropriate categories
- fixing necessary details

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

Depends on sagemath#37778
    
URL: sagemath#37719
Reported by: Frédéric Chapoton
Reviewer(s): Matthias Köppe, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
    
A somewhat large refactoring of some of the auld classes for rings, and
related categories

- introducing a new category of Noetherian rings
- moving some methods in appropriate categories
- fixing necessary details

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

Depends on sagemath#37778
    
URL: sagemath#37719
Reported by: Frédéric Chapoton
Reviewer(s): Matthias Köppe, Travis Scrimshaw
@vbraun vbraun merged commit 6bccaa3 into sagemath:develop May 12, 2024
15 checks passed
@fchapoton fchapoton deleted the refactor_ring_categories branch May 12, 2024 19:56
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 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.

6 participants