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:

Status badges

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 ghseeli

  • Branch set to u/ghseeli/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra

comment:2 Changed 7 years ago by ghseeli

  • Commit set to 28ee71f199a7162aa57408864e451f8a3477fcbf

Last 10 new commits:

3a47198created a BrauerDiagram class with specific BrauerDiagram methods.
496382aadded long time to long doc tests and other minor documentation changes.
6ba0605added 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.
37fa5e0added documentation for Brauer diagram compact_repr
af7f86cMerge 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
52d11e5Added morphism to SymmetricGroupAlgebra to DiagramAlgebra and registered it as coercion
08a04a7Merge branch 'develop' into t/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator
35b7663Merge 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
169e666added missing import statement to diagram algebras file
28ee71ffixed the order of commands and import statements

comment:3 Changed 7 years ago by ghseeli

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 git

  • Commit changed from 28ee71f199a7162aa57408864e451f8a3477fcbf to 69bd5dbdd4cb11875dc5ab79cd3a8735633f3226

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

69bd5dbchanged 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 tscrim

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 ghseeli

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 git

  • Commit changed from 69bd5dbdd4cb11875dc5ab79cd3a8735633f3226 to 22c7116a9e7355b0b351a9d7dd733e8423fcdcf2

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

d16a7f5tried moving SymmetricGroupAlgebra declaration in DiagramAlgebras init to no avail
22c7116temporary workaround for getting SymmetricGroupAlgebra initialized by initializing twice.

comment:8 Changed 7 years ago by tscrim

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 git

  • Commit changed from 22c7116a9e7355b0b351a9d7dd733e8423fcdcf2 to e138144a3975a51a005f06df98c6ef1b0fad1753

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

5b48761Merge 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
cb07038removed category declaration and unnecessary comment
145d421Merge branch 'develop' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
4af2b92Merge branch 'develop' into t/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator
8a8b0cfChanged all 'returns' in the documentation to 'return'
c97973aadded more doctests to methods that did not have them
0ca1903Added more doctests for cardinality and element_constructor methods
6ba9379added a few more doctests for a few methods I missed
6339baaMerge 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
e138144Added documentation and a doctest for coercion

comment:10 Changed 7 years ago by ghseeli

  • Milestone changed from sage-6.8 to sage-6.9

comment:11 Changed 7 years ago by git

  • Commit changed from e138144a3975a51a005f06df98c6ef1b0fad1753 to d4d3a091c747cf5ed31f02eb10d6f3298445aecd

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

3d59b52fixed a few typos
e66621fAdded attribution to authors in source
4aab0cachanged major typo in AbstractDiagrams initializer
01c0c04Doing some cleanup and adding a global options.
d424bcfMerge branch 'u/ghseeli/diagram_algebra_improvements-18720' of trac.sagemath.org:sage into u/tscrim/diagram_algebra_improvements-18720
298ff6fRemoving some unnecessary imports.
b15960dFixed typo in documentation.
489b189fixed a doctest that was oddly passing but should not have. See commit details
51e2984manual merge. I hope I got all the imports right
d4d3a09accidentally deleted crucial statement for coercion. I have reinserted it.

comment:12 Changed 7 years ago by ghseeli

  • Status changed from new to needs_review

comment:13 Changed 7 years ago by git

  • Commit changed from d4d3a091c747cf5ed31f02eb10d6f3298445aecd to 2adcfeffa5d87f137ee9010fb69087a2b76e98a7

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

c716ed1Merge branch 'develop' into t/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator
fa3f792Fixed doctest in sources.py.
616b818Fixing the documentation and some code cleanup/python3 support.
7160ffefixed merge conflicts
2adcfefoops! Didn't completely merge

comment:14 Changed 7 years ago by tscrim

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: Changed 7 years ago by vdelecroix

  • 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 tscrim

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: Changed 7 years ago by ghseeli

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 vdelecroix

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 git

  • Commit changed from 2adcfeffa5d87f137ee9010fb69087a2b76e98a7 to 6e7cb2173baacb6b5bbb8b29ec7cafb57c4ad6c7

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

781c643Merge branch 'develop' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
888f1a2Merge branch 'develop' into t/18762/create_coercion_between_diagram_algebras_and_the_symmetric_group_algebra
6e7cb21Created conversions of SymmetricGroupAlgebra to DiagramAlgebras as well as appropriate coercions where it makes sense

comment:20 Changed 7 years ago by ghseeli

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 tscrim

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 git

  • Commit changed from 6e7cb2173baacb6b5bbb8b29ec7cafb57c4ad6c7 to ea857ff335ac439123ef7b7891bdd8909e5c3fb4

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

ea857ffadded 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 ghseeli

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 tscrim

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 git

  • Commit changed from ea857ff335ac439123ef7b7891bdd8909e5c3fb4 to a4c199d2842865c6385e6938b67974f39ebd8876

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

a4c199dadded the necessary extra colon after tests

comment:26 Changed 7 years ago by tscrim

  • 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:

14f80d3Reviewer changes and fixing coercions.

comment:27 Changed 7 years ago by ghseeli

  • Status changed from needs_review to positive_review

comment:28 Changed 7 years ago by ghseeli

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 tscrim

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 vbraun

  • Branch changed from u/tscrim/diagram_algebras_SGA-18762 to 14f80d3624d95079c896cb5efe8f29988834e1ac
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.