Opened 7 years ago
Closed 6 years ago
#16219 closed enhancement (fixed)
Implement a catalog for algebras
Reported by:  tscrim  Owned by:  tscrim 

Priority:  major  Milestone:  sage6.4 
Component:  interfaces  Keywords:  algebras catalog 
Cc:  jhpalmieri, ncohen  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  1a8a288 (Commits, GitHub, GitLab)  Commit:  1a8a28854c82086cf7e0b8eb73d694eeecb52fc9 
Dependencies:  #16508  Stopgaps: 
Description
Like groups, crystals, etc.
Change History (21)
comment:1 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:2 Changed 7 years ago by
 Branch set to public/algebras/catalog16219
 Commit set to c951590b5659c5cd3f5f9343792a56b8126f9d3b
 Status changed from new to needs_review
comment:3 Changed 7 years ago by
 Commit changed from c951590b5659c5cd3f5f9343792a56b8126f9d3b to 61d6bd1dd3be8ef99ac8c835fd94890e68bef235
Branch pushed to git repo; I updated commit sha1. New commits:
61d6bd1  Added quaternion algebra to doc and some cleanup of the file.

comment:4 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:5 followup: ↓ 9 Changed 6 years ago by
I have just been through the diff, and overall it looks good.
It seems a bit redundant to both import the algebras and link to their doc. We should find a better way to handle this automatically at some point. But that's for a later ticket.
The documentation improvements elsewhere in the algebra module is nice. Yet's it's orthogonal to the main purpose of the ticket. Next time, please make it a separate ticket.
Why not lazy importing everything?
Up to this last point, I believe it's good to go (assuming all tests pass).
comment:6 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:7 Changed 6 years ago by
 Reviewers set to Nicolas M. Thiéry
comment:8 Changed 6 years ago by
Ah, also: at some point, we will want to point to all the Hopf algebras from combinat. But it's probably easier to do this once #16256 is done.
comment:9 in reply to: ↑ 5 ; followup: ↓ 10 Changed 6 years ago by
Replying to nthiery:
I have just been through the diff, and overall it looks good.
Thanks for taking a look at this.
It seems a bit redundant to both import the algebras and link to their doc. We should find a better way to handle this automatically at some point. But that's for a later ticket.
Concur.
The documentation improvements elsewhere in the algebra module is nice. Yet's it's orthogonal to the main purpose of the ticket. Next time, please make it a separate ticket.
When I added the quaternion_algebra.py
to the doc, it causes docbuild failures. The cleanup was just as I was going through reading the file in order for it to build.
Why not lazy importing everything?
I lazy imported things that were lazily imported into the global namespace, so it wouldn't trigger an import when doing tab completion on algebras.
(as what happens for groups).
There's a more trickier question of what algebras could we remove from the global namespace (it's not nearly as clear as it was for crystals to me), but I think that can be done on a followup.
Up to this last point, I believe it's good to go (assuming all tests pass).
Was this suppose to be set as needs_info? I don't quite understand why this was set to needs_work.
Ah, also: at some point, we will want to point to all the Hopf algebras from combinat. But it's probably easier to do this once #16256 is done.
Yep, and it should probably be a subcatalog called combinatorial_hopf
(or maybe just .hopf
and include all Hopf algebras). Anyways, stuff for later and I'll take a look at #16256 soon.
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 6 years ago by
Replying to tscrim:
Why not lazy importing everything?
I lazy imported things that were lazily imported into the global namespace, so it wouldn't trigger an import when doing tab completion on
algebras.
(as what happens for groups). Was this suppose to be set as needs_info? I don't quite understand why this was set to needs_work.
I was thinking of lazy importing everything here right away. Maybe using a flag "at_startup" to make the point that those should really be lazy imported elsewhere as well. What do you think?
There's a more trickier question of what algebras could we remove from the global namespace (it's not nearly as clear as it was for crystals to me), but I think that can be done on a followup.
+1
Yep, and it should probably be a subcatalog called
combinatorial_hopf
(or maybe just.hopf
and include all Hopf algebras). Anyways, stuff for later and I'll take a look at #16256 soon.
And at some point we will want to automatize the building of all those
catalogs. E.g. following the prototype Florent wrote during the Sage
days in Edinburgh that instruments TestSuite
to build a database of
all parents with their category, from which we can get all parents in
a category by reverse lookup. I just created #17219 for this.
Cheers,
Nicolas
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 6 years ago by
Replying to nthiery:
I was thinking of lazy importing everything here right away. Maybe using a flag "at_startup" to make the point that those should really be lazy imported elsewhere as well. What do you think?
So do you mean you want everything to be lazily imported or do you want to lazy import the catalog? If you want the latter, this causes problems because then the imports are triggered on tab completion (you can see this occur with the groups.<tab>
).
comment:12 in reply to: ↑ 11 Changed 6 years ago by
Replying to tscrim:
So do you mean you want everything to be lazily imported or do you want to lazy import the catalog? If you want the latter, this causes problems because then the imports are triggered on tab completion (you can see this occur with the
groups.<tab>
).
The former: lazy importing everything that is in the catalog.
comment:13 Changed 6 years ago by
 Commit changed from 61d6bd1dd3be8ef99ac8c835fd94890e68bef235 to 850bebb5bbf92322178bacddd68c7f89e0ca0e3b
Branch pushed to git repo; I updated commit sha1. New commits:
850bebb  Merge branch 'public/algebras/catalog16219' of trac.sagemath.org:sage into public/algebras/catalog16219

comment:14 Changed 6 years ago by
 Status changed from needs_work to needs_review
IMO, we should not lazy import things which are (currently) not lazily imported into the global namespace as it is more coherent. I think we should have a (separate) discussion about which algebras should be lazily imported and handle it there.
comment:15 Changed 6 years ago by
I am not convinced, but I don't have a strong opinion either, and it's minor. I won't hold up this ticket just for this. Please proceed if you are convinced this is the way to go.
comment:16 Changed 6 years ago by
 Status changed from needs_review to positive_review
I believe it is, and I'm interpreting that as a positive review. I've created #17271 for the lazy importing. Thanks Nicolas.
comment:17 Changed 6 years ago by
 Commit changed from 850bebb5bbf92322178bacddd68c7f89e0ca0e3b to 1a8a28854c82086cf7e0b8eb73d694eeecb52fc9
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:
a4d5b24  Cached methods should return immutable matrices

aae8f5b  Merge branch 'u/jhpalmieri/DGA_new' of trac.sagemath.org:sage into public/algebras/cdga16508

7b6f118  Fixed doctests.

fb16080  Merge branch 'develop' into public/algebras/cdga16508

b8328e6  Second pass of reworking things around by adding functionality.

d732dbe  Commutative DGAs: rename some classes

497d9d6  Merge branch 'public/algebras/cdga16508' of trac.sagemath.org:sage into public/algebras/cdga16508

3900c66  Renamed CDGAlgebra > cdg_algebra.

73ee329  Merge branch 'public/algebras/catalog16219' of trac.sagemath.org:sage into public/algebras/cdga16508

1a8a288  Added GradedCommutative to the algebras catalog.

comment:18 Changed 6 years ago by
 Dependencies set to #16508
I've added #16508 to the catalog and moved DifferentialWeyl
to it's correct spot in the catalog (it changed its name from Weyl
). Very quick check needed.
comment:19 followup: ↓ 20 Changed 6 years ago by
 Status changed from needs_review to positive_review
Check done. Ok for me!
Just for the record: the comment for 73ee329 looks backward ...
comment:20 in reply to: ↑ 19 Changed 6 years ago by
comment:21 Changed 6 years ago by
 Branch changed from public/algebras/catalog16219 to 1a8a28854c82086cf7e0b8eb73d694eeecb52fc9
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Initial catalog of algebras.
Merge branch 'develop' into public/algebras/catalog16219
Merge branch 'develop' into public/algebras/catalog16219
Added some more algebras.