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

Priority:  major  Milestone:  sage6.7 
Component:  algebra  Keywords:  algebras, categories 
Cc:  nthiery, tscrim  Merged in:  
Authors:  Aladin Virmaux  Reviewers:  Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  6a7b4f5 (Commits)  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 5 years ago by
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 5 years ago by
 Branch set to u/virmaux/t/default_behaviour_algebra_generators
comment:3 Changed 5 years ago by
 Commit set to ceb8afd50e10cb316ba2db86310ce4b7390ba434
comment:4 Changed 5 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 5 years ago by
 Status changed from new to needs_review
Oops, indeed, it does work (see the exemples in the code).
comment:6 Changed 5 years ago by
 Branch changed from u/virmaux/t/default_behaviour_algebra_generators to u/nthiery/t/default_behaviour_algebra_generators
comment:7 Changed 5 years ago by
 Commit changed from ceb8afd50e10cb316ba2db86310ce4b7390ba434 to 6a7b4f59ae87964d218bae3f75c757421eeb3f85
Branch pushed to git repo; I updated commit sha1. New commits:
6a7b4f5  16659: fixed crosslinks

comment:8 Changed 5 years ago by
 Branch changed from u/nthiery/t/default_behaviour_algebra_generators to u/virmaux/t/default_behaviour_algebra_generators
 Commit changed from 6a7b4f59ae87964d218bae3f75c757421eeb3f85 to ceb8afd50e10cb316ba2db86310ce4b7390ba434
 Reviewers set to Nicolas M. Thiéry
 Status changed from needs_review to positive_review
Thanks!
Proofread and good to go! Aladin double checked my changes over my shoulder.
comment:9 Changed 5 years ago by
 Status changed from positive_review to 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 5 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 5 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 5 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 5 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 5 years ago by
 Branch changed from u/virmaux/t/default_behaviour_algebra_generators to u/nthiery/t/default_behaviour_algebra_generators
comment:15 Changed 5 years ago by
 Commit changed from ceb8afd50e10cb316ba2db86310ce4b7390ba434 to 6a7b4f59ae87964d218bae3f75c757421eeb3f85
 Status changed from needs_work to 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 5 years ago by
 Branch changed from u/nthiery/t/default_behaviour_algebra_generators to 6a7b4f59ae87964d218bae3f75c757421eeb3f85
 Resolution set to fixed
 Status changed from positive_review to 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