some changes in quaternion algebras

sagemath / sage

Main repository of SageMath

https://www.sagemath.org

Other

1.23k stars 429 forks source link

some changes in quaternion algebras #32811

Closed fchapoton closed 2 years ago

fchapoton commented 2 years ago

a bunch of pep8 fixes

also trying not to use ParentWithGens, but only Parent

CC: @tscrim @slel @kliem

Component: algebra

Keywords: pep8, ParentWithGens, Parent

Author: Frédéric Chapoton

Branch/Commit: a0799b7

Reviewer: Jonathan Kliem

Issue created by migration from https://trac.sagemath.org/ticket/32811

fchapoton commented 2 years ago

Branch: u/chapoton/32811

fchapoton commented 2 years ago

Commit: ed8b33d

fchapoton commented 2 years ago

New commits:

ed8b33d some changes in quaternion algebras (use Parent)

fchapoton commented 2 years ago

comment:2

double green, so please review

kliem commented 2 years ago

comment:3

Why use Parent instead of ParentWithGens?

I'm actually wondering why we call some low level init instead of just relying on Algebra.__init__?

fchapoton commented 2 years ago

comment:4

see src/sage/structure/parent_old.pyx and src/sage/structure/parent_base.pyx

In brief, ParentWithGens is part of the "old-coercion-framework", which is still used a lot, in particular in number fields and commutative rings. This is supposed to be deprecated, but removing that is a daunting task.

kliem commented 2 years ago

comment:5

But wouldn't this be resolved by using Algebra as well? Eventually Algebra needs a new base, doesn't it?

fchapoton commented 2 years ago

comment:6

I think one should prefer to use "Parent" and set the appropriate category, rather than using something else than Parent, like Algebra or Ring. But I may be wrong, as nothing is very clear.

kliem commented 2 years ago

comment:7

I see. However what is confusing me is that we are still inheriting from Algebra:

class QuaternionAlgebra_abstract(Algebra):

If this is correct, we should initialize Algebra. If this is not correct, we should just straight inherit from Parent.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from ed8b33d to 6b826c3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`4f18c50`	`Merge branch 'u/chapoton/32811' in 9.5,b7`
`6b826c3`	`trying to use Algebra.__init__`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

a0799b7 fine tuning categories of quaternions

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 6b826c3 to a0799b7

fchapoton commented 2 years ago

comment:11

Hopefully, this looks better now.

Quaternion rings subclass Algebra, and use Algebra.__init__

Quaternion orders subclass Parent and use Parent.__init__

fchapoton commented 2 years ago

comment:12

and the lights are green, so please review

kliem commented 2 years ago

comment:13

Note that the following still works:

sage: from sage.algebras.algebra import is_Algebra
sage: Q = QuaternionAlgebra(-1,-7).maximal_order()
sage: is_Algebra(Q)

kliem commented 2 years ago

Reviewer: Jonathan Kliem

slel commented 2 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 a bunch of pep8 fixes

-also trying not to use ParentWithGens, but only Parent
+also trying not to use `ParentWithGens`, but only `Parent`

slel commented 2 years ago

Changed keywords from none to pep8, ParentWithGens, Parent

slel commented 2 years ago

comment:16

See also #32245 and #32264 about quaternion algebras, and more generally

https://trac.sagemath.org/query?order=id&desc=1&status=!closed&summary=~quaternion

vbraun commented 2 years ago

Changed branch from u/chapoton/32811 to a0799b7