Opened 7 years ago
Closed 7 years ago
#18336 closed enhancement (fixed)
Give a default behavious to algebra_generators
Reported by:  Aladin Virmaux  Owned by:  

Priority:  major  Milestone:  sage6.7 
Component:  algebra  Keywords:  algebras, categories 
Cc:  Nicolas M. Thiéry, Travis Scrimshaw  Merged in:  
Authors:  Aladin Virmaux  Reviewers:  Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  6a7b4f5 (Commits, GitHub, GitLab)  Commit:  6a7b4f59ae87964d218bae3f75c757421eeb3f85 
Dependencies:  Stopgaps: 
Description (last modified by )
Give a default behaviour to the method algebra_generators
for algebras_with_basis
by calling basis()
.
Change History (16)
comment:1 Changed 7 years ago by
Type:  PLEASE CHANGE → enhancement 

comment:2 Changed 7 years ago by
Branch:  → u/virmaux/t/default_behaviour_algebra_generators 

comment:3 Changed 7 years ago by
Commit:  → ceb8afd50e10cb316ba2db86310ce4b7390ba434 

comment:4 Changed 7 years ago by
Description:  modified (diff) 

Is this ready for review? Also without the patch, you're able to create the descent algebra, correct (i.e., your missing the call to algebra_generators
in your example)?
comment:5 Changed 7 years ago by
Status:  new → needs_review 

Oops, indeed, it does work (see the exemples in the code).
comment:6 Changed 7 years ago by
Branch:  u/virmaux/t/default_behaviour_algebra_generators → u/nthiery/t/default_behaviour_algebra_generators 

comment:7 Changed 7 years ago by
Commit:  ceb8afd50e10cb316ba2db86310ce4b7390ba434 → 6a7b4f59ae87964d218bae3f75c757421eeb3f85 

Branch pushed to git repo; I updated commit sha1. New commits:
6a7b4f5  16659: fixed crosslinks

comment:8 Changed 7 years ago by
Branch:  u/nthiery/t/default_behaviour_algebra_generators → u/virmaux/t/default_behaviour_algebra_generators 

Commit:  6a7b4f59ae87964d218bae3f75c757421eeb3f85 → ceb8afd50e10cb316ba2db86310ce4b7390ba434 
Reviewers:  → Nicolas M. Thiéry 
Status:  needs_review → positive_review 
Thanks!
Proofread and good to go! Aladin double checked my changes over my shoulder.
comment:9 Changed 7 years ago by
Status:  positive_review → needs_work 

sage t long src/sage/categories/algebras.py ********************************************************************** File "src/sage/categories/algebras.py", line 160, in sage.categories.algebras.Algebras.Quotients.ParentMethods.algebra_generators Failed example: S.algebra_generators() Expected: Finite family {'y': B['y'], 'x': B['x'], 'b': 0, 'a': 0} Got: Finite family {'y': B['y'], 'x': B['x']} ********************************************************************** 1 item had failures: 1 of 4 in sage.categories.algebras.Algebras.Quotients.ParentMethods.algebra_generators [24 tests, 1 failure, 0.32 s]
comment:10 Changed 7 years ago by
Umm is it a "bug" from Algebras.Quotients ? Here is the line to get the algebra generators of the quotient:
return self.ambient().algebra_generators().map(self.retract)
and we get generators that are 0
...
But this methods should have been called instead of the newly implemented one, am I missing something ?
comment:11 Changed 7 years ago by
Algebras.WithBasis
and Algebras.Quotients
are independent of each other, so when you inherit from both there is an ambiguity. My guess is that Algebras.WithBasis
ends up being before Algebras.Quotients
in the categories of S
, so the "wrong" one is picked. So we need to manually resolve this ambiguity. I'll have a look.
comment:12 Changed 7 years ago by
For info: the tests pass here; so this might depend on the context, which indeed calls for resolving the conflict.
Volker: any idea if this fails on all machines or only from time to time? Or maybe when this patch is merged together with #16659?
comment:13 Changed 7 years ago by
I didn't have #16659 merged when I tested this ticket, and I don't spot any obviouslyconflicting ticket before that. Maybe its easiest to wait for the next beta if you can't reproduce it.
comment:14 Changed 7 years ago by
Branch:  u/virmaux/t/default_behaviour_algebra_generators → u/nthiery/t/default_behaviour_algebra_generators 

comment:15 Changed 7 years ago by
Commit:  ceb8afd50e10cb316ba2db86310ce4b7390ba434 → 6a7b4f59ae87964d218bae3f75c757421eeb3f85 

Status:  needs_work → positive_review 
Oops, I see what was the issue! There was a conflict when I concurrently pushed my branch to trac and commented on the ticket, and I accidently reset the branch to the previous one. We were lucky that the old one did not work so that this got noticed!
This is fixed now. So back to positive review. Sorry for the mess Volker!
New commits:
46f4f54  18336: moved default implementation of algebra_generators to MagmaticAlgebras.WithBasis + proofread doc

6a7b4f5  16659: fixed crosslinks

comment:16 Changed 7 years ago by
Branch:  u/nthiery/t/default_behaviour_algebra_generators → 6a7b4f59ae87964d218bae3f75c757421eeb3f85 

Resolution:  → fixed 
Status:  positive_review → closed 
Namely we return the basis as a family of generators.
Without patch:
With patch:
It does not override specific class methods:
New commits:
18336: algebra_generators in algebras_with_basis