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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

use CommutativeRing in ring_extension #37316

Merged
merged 1 commit into from
Feb 25, 2024

Conversation

fchapoton
Copy link
Contributor

Removing one of the last uses of CommutativeAlgebra class, towards its deprecation.

Also a few pep8 fixes in the modified file.

馃摑 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.

Copy link

Documentation preview for this PR (built with commit 8de95f8; changes) is ready! 馃帀

@mantepse
Copy link
Collaborator

Does this actually mean that inheriting from CommutativeAlgebra doesn't do anything beyond making

sage: isinstance(K, CommutativeAlgebra)
True

?

More precisely, I tried:

sage: F = GF(5^2)
sage: k = GF(5^4)
sage: z4 = k.gen()
sage: K.<a> = k.over(F, gen=1-z4); K
Field in a with defining polynomial x^2 + z2*x + 4 over its base
sage: K in Algebras(K.base_ring())
False
sage: from sage.rings.ring import CommutativeAlgebra
sage: isinstance(K, CommutativeAlgebra)
True

and with the new branch, the last line is False (which looks completely OK, of course, I am just surprised).

(I admit that I'm not completely sure what ring_extension.pyx actually does)

@fchapoton
Copy link
Contributor Author

yes, probably your are right. The usage of isinstance should be avoided and categories should be used instead.

My aim for now is to keep only the bad class CommutativeRing and deprecate several others auld-parent classes like Algebra, CommutativeAlgebra, etc. Then and only then, I will try to disentangle what happens for all commutative rings (and this will be a huge mess)

The change here is a step towards deprecating CommutativeAlgebra, which is almost no longer used already.

Copy link
Collaborator

@mantepse mantepse 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!

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2024
    
Removing one of the last uses of `CommutativeAlgebra` class, towards its
deprecation.

Also a few pep8 fixes in the modified file.

### 馃摑 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#37316
Reported by: Fr茅d茅ric Chapoton
Reviewer(s): Martin Rubey
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 19, 2024
    
Removing one of the last uses of `CommutativeAlgebra` class, towards its
deprecation.

Also a few pep8 fixes in the modified file.

### 馃摑 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#37316
Reported by: Fr茅d茅ric Chapoton
Reviewer(s): Martin Rubey
@vbraun vbraun merged commit f016b01 into sagemath:develop Feb 25, 2024
24 checks passed
@fchapoton fchapoton deleted the commring_ring_extension branch February 25, 2024 12:21
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants