sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.39k stars 472 forks source link

Implement a catalog for algebras #16219

Closed tscrim closed 9 years ago

tscrim commented 10 years ago

Like groups, crystals, etc.

Depends on #16508

CC: @jhpalmieri @nathanncohen

Component: interfaces

Keywords: algebras catalog

Author: Travis Scrimshaw

Branch/Commit: 1a8a288

Reviewer: Nicolas M. Thiéry

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

tscrim commented 10 years ago

Commit: c951590

tscrim commented 10 years ago

Author: Travis Scrimshaw

tscrim commented 10 years ago

New commits:

e1373c9Initial catalog of algebras.
936590dMerge branch 'develop' into public/algebras/catalog-16219
fe1850cMerge branch 'develop' into public/algebras/catalog-16219
c951590Added some more algebras.
tscrim commented 10 years ago

Branch: public/algebras/catalog-16219

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

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

61d6bd1Added quaternion algebra to doc and some cleanup of the file.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from c951590 to 61d6bd1

nthiery commented 9 years ago
comment:5

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).

nthiery commented 9 years ago

Reviewer: Nicolas M. Thiéry

nthiery commented 9 years ago
comment:8

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.

tscrim commented 9 years ago
comment:9

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.

nthiery commented 9 years ago
comment:10

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

tscrim commented 9 years ago
comment:11

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>).

nthiery commented 9 years ago
comment:12

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.

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

Changed commit from 61d6bd1 to 850bebb

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

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

850bebbMerge branch 'public/algebras/catalog-16219' of trac.sagemath.org:sage into public/algebras/catalog-16219
tscrim commented 9 years ago
comment:14

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.

nthiery commented 9 years ago
comment:15

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.

tscrim commented 9 years ago
comment:16

I believe it is, and I'm interpreting that as a positive review. I've created #17271 for the lazy importing. Thanks Nicolas.

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

Changed commit from 850bebb to 1a8a288

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

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:

a4d5b24Cached methods should return immutable matrices
aae8f5bMerge branch 'u/jhpalmieri/DGA_new' of trac.sagemath.org:sage into public/algebras/cdga-16508
7b6f118Fixed doctests.
fb16080Merge branch 'develop' into public/algebras/cdga-16508
b8328e6Second pass of reworking things around by adding functionality.
d732dbeCommutative DGAs: rename some classes
497d9d6Merge branch 'public/algebras/cdga-16508' of trac.sagemath.org:sage into public/algebras/cdga-16508
3900c66Renamed CDGAlgebra -> cdg_algebra.
73ee329Merge branch 'public/algebras/catalog-16219' of trac.sagemath.org:sage into public/algebras/cdga-16508
1a8a288Added GradedCommutative to the algebras catalog.
tscrim commented 9 years ago
comment:18

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.

tscrim commented 9 years ago

Dependencies: #16508

nthiery commented 9 years ago
comment:19

Check done. Ok for me!

Just for the record: the comment for 73ee329 looks backward ...

tscrim commented 9 years ago
comment:20

Replying to @nthiery:

Just for the record: the comment for 73ee329 looks backward ...

I was going to push this to #16508 until I noticed the out-of-order DifferentialWeyl, so in a sense it is backwards. ^^;;

vbraun commented 9 years ago

Changed branch from public/algebras/catalog-16219 to 1a8a288