Opened 7 years ago
Closed 7 years ago
#18762 closed enhancement (fixed)
Create coercion between diagram algebras and the symmetric group algebra
Reported by: | ghseeli | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.10 |
Component: | algebra | Keywords: | diagram algebra, partition algebra, days65 |
Cc: | alauve, s.r.doty, saliola, tscrim | Merged in: | |
Authors: | George H. Seelinger | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 14f80d3 (Commits, GitHub, GitLab) | Commit: | 14f80d3624d95079c896cb5efe8f29988834e1ac |
Dependencies: | #18720 | Stopgaps: |
Description
There is a natural embedding of elements of the symmetric group algebra into diagram algebras, especially the partition algebra and the Brauer algebra. Therefore, there should be a morphism registered to the coercion model to allow for symmetric group algebra elements to be coerced into the appropriate diagram algebras. This morphism is pretty straight forward and will only take a few lines to implement.
Change History (30)
comment:1 Changed 7 years ago by
- Branch set to u/ghseeli/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
comment:2 Changed 7 years ago by
- Commit set to 28ee71f199a7162aa57408864e451f8a3477fcbf
comment:3 Changed 7 years ago by
Alright, so, the code at present does not work. For some reason, I get an MRO error when I try to initialize the SymmetricGroupAlgebra? with the same base ring as the DiagramAlgebra?. Does anyone have any suggestions about how to fix this?
comment:4 Changed 7 years ago by
- Commit changed from 28ee71f199a7162aa57408864e451f8a3477fcbf to 69bd5dbdd4cb11875dc5ab79cd3a8735633f3226
Branch pushed to git repo; I updated commit sha1. New commits:
69bd5db | changed the ring of the symmetric group algebra created for the morphism from sga to diagram algebra to be the base ring of the diagram algebra. However, this causes a type error with the MRO
|
comment:5 Changed 7 years ago by
An MRO error is extremely strange. I will take a look at this as soon as I can.
comment:6 Changed 7 years ago by
Is this a bug in the SymmetricGroupAlgebra??
sage: SymmetricGroupAlgebra(SR,3,category = FiniteDimensionalAlgebrasWithBasis(SR)) Traceback ... TypeError: Cannot create a consistent method resolution order (MRO) for bases MagmaticAlgebras.subcategory_class, UnitalAlgebras.WithBasis.subcategory_class, AdditiveMagmas.Algebras.subcategory_class
But, in a completely new Sage session:
sage: SymmetricGroupAlgebra(SR,3) Symmetric group algebra of order 3 over Symbolic Ring sage: SymmetricGroupAlgebra(SR,3,category = FiniteDimensionalAlgebrasWithBasis(SR)) Symmetric group algebra of order 3 over Symbolic Ring
I have no idea what could be going on here. Is it some clever caching that somehow makes things work? In any event, this is the cause of the issue, I think.
(I tried this in sage6.8beta5)
comment:7 Changed 7 years ago by
- Commit changed from 69bd5dbdd4cb11875dc5ab79cd3a8735633f3226 to 22c7116a9e7355b0b351a9d7dd733e8423fcdcf2
comment:8 Changed 7 years ago by
Oh wow, that's very interesting behavior...However it is not quite proper input as the category of the Weyl group W
. Yet it should behave correctly and consistently, or at least give a better error message.
comment:9 Changed 7 years ago by
- Commit changed from 22c7116a9e7355b0b351a9d7dd733e8423fcdcf2 to e138144a3975a51a005f06df98c6ef1b0fad1753
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
5b48761 | Merge branch 't/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
|
cb07038 | removed category declaration and unnecessary comment
|
145d421 | Merge branch 'develop' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
|
4af2b92 | Merge branch 'develop' into t/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator
|
8a8b0cf | Changed all 'returns' in the documentation to 'return'
|
c97973a | added more doctests to methods that did not have them
|
0ca1903 | Added more doctests for cardinality and element_constructor methods
|
6ba9379 | added a few more doctests for a few methods I missed
|
6339baa | Merge branch 't/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
|
e138144 | Added documentation and a doctest for coercion
|
comment:10 Changed 7 years ago by
- Milestone changed from sage-6.8 to sage-6.9
comment:11 Changed 7 years ago by
- Commit changed from e138144a3975a51a005f06df98c6ef1b0fad1753 to d4d3a091c747cf5ed31f02eb10d6f3298445aecd
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
3d59b52 | fixed a few typos
|
e66621f | Added attribution to authors in source
|
4aab0ca | changed major typo in AbstractDiagrams initializer
|
01c0c04 | Doing some cleanup and adding a global options.
|
d424bcf | Merge branch 'u/ghseeli/diagram_algebra_improvements-18720' of trac.sagemath.org:sage into u/tscrim/diagram_algebra_improvements-18720
|
298ff6f | Removing some unnecessary imports.
|
b15960d | Fixed typo in documentation.
|
489b189 | fixed a doctest that was oddly passing but should not have. See commit details
|
51e2984 | manual merge. I hope I got all the imports right
|
d4d3a09 | accidentally deleted crucial statement for coercion. I have reinserted it.
|
comment:12 Changed 7 years ago by
- Status changed from new to needs_review
comment:13 Changed 7 years ago by
- Commit changed from d4d3a091c747cf5ed31f02eb10d6f3298445aecd to 2adcfeffa5d87f137ee9010fb69087a2b76e98a7
Branch pushed to git repo; I updated commit sha1. New commits:
c716ed1 | Merge branch 'develop' into t/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator
|
fa3f792 | Fixed doctest in sources.py.
|
616b818 | Fixing the documentation and some code cleanup/python3 support.
|
7160ffe | fixed merge conflicts
|
2adcfef | oops! Didn't completely merge
|
comment:14 Changed 7 years ago by
This currently breaks the requirements of a coercion in that the coercion map must be defined on all elements of the domain. So it will need to be a conversion, which you can implement in the _element_constructor_
method by
if isinstance(x, SymmetricGroupAlgebra.Element) and other_conditions_on_parent(x): return convert_to_element_of_self(x)
For those diagram algebras which this is well-defined on the entire domain, you can make the call to self
a coercion in the constructor or implement:
def _coerce_map_from_(self, P): if isinstance(P, SymmetricGroupAlgebra) and other_conditions(P): return True return super(self, DiagramAlgebra)._coerce_map_from_(P)
comment:15 follow-up: ↓ 16 Changed 7 years ago by
- Status changed from needs_review to needs_work
Hello,
Just some sided remarks.
- You have more commits than the number of lines you modified! Moreover most of your commit messages are completely useless. The git history is important since it is used to find out which commit is responsible for a given change. You would better start a new (clean) branch from scratch.
- methods must be documented (see the developer guide)
Vincent
comment:16 in reply to: ↑ 15 Changed 7 years ago by
Replying to vdelecroix:
- You have more commits than the number of lines you modified! Moreover most of your commit messages are completely useless. The git history is important since it is used to find out which commit is responsible for a given change. You would better start a new (clean) branch from scratch.
This comment is completely off base. First, learn to use git blame
and git log filename
(or git log -p filename
). Second, the commit messages tell about the changes (although there are some of them that are really long; git practices want the first lines to be short). You are being way too precious about the git history.
comment:17 follow-up: ↓ 18 Changed 7 years ago by
Vincent,
Thank you for your feedback. I had intended to change the status from needs_review to needs_work when Travis made his comments 3 weeks ago, so I am sorry you felt compelled to review something that was already in need of a lot of work. I understand that methods must be documented and I will do so accordingly before there is a positive review. However, first, I needed to establish what is going on here, (see the earlier comments about weird coercion behavior) and that is still somewhat (?) in the air. I do agree that my git comments are rather terse, but at the same time, as you note, most of them are rather small and thus looking at the diff should be more than enough to get an idea of what is happening. Furthermore, as someone who develops software semi-professionally, it is not unusual that the number of commits exceeds the number of lines changed. Most software developers would say that the number of lines affected has little correlation with content. Finally, a lot of these commits are due to the age of the ticket. New versions of the beta need to be merged into the branch so that the feature doesn't depart from the current beta. I am sorry that my development cycle has been so slow that this has been necessary. I hope to finish this ticket soon. Thank you for your feedback and I hope you will be more satisfied with the end result when it goes back up for review.
Best, George
comment:18 in reply to: ↑ 17 Changed 7 years ago by
Hello,
Replying to ghseeli:
Finally, a lot of these commits are due to the age of the ticket. New versions of the beta need to be merged into the branch so that the feature doesn't depart from the current beta.
There is no need to merge the last beta.
But anyway, the most important is to do how you feel. And take for the best other's comments to get a better Sage software.
Vincent
comment:19 Changed 7 years ago by
- Commit changed from 2adcfeffa5d87f137ee9010fb69087a2b76e98a7 to 6e7cb2173baacb6b5bbb8b29ec7cafb57c4ad6c7
Branch pushed to git repo; I updated commit sha1. New commits:
781c643 | Merge branch 'develop' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
|
888f1a2 | Merge branch 'develop' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
|
6e7cb21 | Created conversions of SymmetricGroupAlgebra to DiagramAlgebras as well as appropriate coercions where it makes sense
|
comment:20 Changed 7 years ago by
Alright, I have now added conversions instead of coercions where appropriate. Furthermore, I have included some appropriate unit tests. I was wondering, should I also include some negative unit tests? As in, verify an error is raised when someone tries to instantiate an invalid algebra element using an inappropriate symmetric group algebra element?
Thank you!
comment:21 Changed 7 years ago by
Yes, these are also useful to verify that you are checking things properly. Also you should make sure to test corner cases.
comment:22 Changed 7 years ago by
- Commit changed from 6e7cb2173baacb6b5bbb8b29ec7cafb57c4ad6c7 to ea857ff335ac439123ef7b7891bdd8909e5c3fb4
Branch pushed to git repo; I updated commit sha1. New commits:
ea857ff | added more negative unit tests for conversion from symmetric group algebra to diagram algebra. One test does not pass
|
comment:23 Changed 7 years ago by
Alright, I added some negative unit tests. However, I am now having trouble getting the following test to pass:
sage: R.<x> = QQ[] sage: BA = BrauerAlgebra(2, x, R) sage: BA.ambient().has_coerce_map_from(BA) True
However, when I run these commands in Sage, the result comes out as expected. Any ideas on why this is happening?
comment:24 Changed 7 years ago by
My guess is that we have the coercion situation BA -> X -> A
where X
is some intermediate object that makes the coercion chain. At some point in the doctests:
1 - BA
and A
have been created, but X
has not.
2 - There is a check in some way for if there is a coercion from BA
to A
, which there is not. (This can be something as innocuous as A(x)
where x in BA
).
3 - The result of 2 is cached, so even when X
is created, there is no check to see if there is a coercion path from BA
to A
.
However that is just a quick guess from the symptoms. I will take a more detailed look over the next few days.
Side note in case you didn't notice, you have TESTS:
block which should be TESTS::
.
comment:25 Changed 7 years ago by
- Commit changed from ea857ff335ac439123ef7b7891bdd8909e5c3fb4 to a4c199d2842865c6385e6938b67974f39ebd8876
Branch pushed to git repo; I updated commit sha1. New commits:
a4c199d | added the necessary extra colon after tests
|
comment:26 Changed 7 years ago by
- Branch changed from u/ghseeli/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra to u/tscrim/diagram_algebras_SGA-18762
- Commit changed from a4c199d2842865c6385e6938b67974f39ebd8876 to 14f80d3624d95079c896cb5efe8f29988834e1ac
- Milestone changed from sage-6.9 to sage-6.10
- Priority changed from minor to major
- Reviewers set to Travis Scrimshaw
- Status changed from needs_work to needs_review
Okay, so I figured out what was going on.
<technical>What was happening was you were creating a coercion from a subalgebra to the ambient algebra, but the ambient algebra could be garbage collected because the morphism (which only holds a strong reference to the codomain) could be collected and the subalgebra didn't have a strong reference to the ambient algebra. When the ambient algebra was recreated, the morphism was not, and so there was no coercion registered (which is done in the codomains). I reworked things so the lift morphism was strongly referenced, and hence the ambient algebra, by the subalgebra.</technical>
I also reworked the morphisms to the Sage standard practice and expanded coercions to allow when there is also a coercion of base rings. There were also some other misc issues that I fixed as well.
If you're happy with my changes, then you can set a positive review.
New commits:
14f80d3 | Reviewer changes and fixing coercions.
|
comment:27 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:28 Changed 7 years ago by
Alright, I now set the ticket to a positive review. Thank you again for all your help Travis! I feel like I am learning a lot more about the internals of Sage.
comment:29 Changed 7 years ago by
Thanks for your contributions. Feel free to cc me on any future tickets you work on (I suspect I will find them to be useful).
comment:30 Changed 7 years ago by
- Branch changed from u/tscrim/diagram_algebras_SGA-18762 to 14f80d3624d95079c896cb5efe8f29988834e1ac
- Resolution set to fixed
- Status changed from positive_review to closed
Last 10 new commits:
created a BrauerDiagram class with specific BrauerDiagram methods.
added long time to long doc tests and other minor documentation changes.
added compact representation for BrauerDiagrams. While this idea can probably be extended to some sub-algebras of the Brauer algebra, this form is based off of Graham--Lehr's bipartition triple form (see the documentation for bipartition_triple in class:BrauerDiagram) which is defined for Brauer algebras.
added documentation for Brauer diagram compact_repr
Merge branch 't/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
Added morphism to SymmetricGroupAlgebra to DiagramAlgebra and registered it as coercion
Merge branch 'develop' into t/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator
Merge branch 't/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
added missing import statement to diagram algebras file
fixed the order of commands and import statements