Create coercion between diagram algebras and the symmetric group algebra

[ghseeli]

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.

[ghseeli]

Last 10 new commits:

​3a47198	created a BrauerDiagram class with specific BrauerDiagram methods.
​496382a	added long time to long doc tests and other minor documentation changes.
​6ba0605	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.
​37fa5e0	added documentation for Brauer diagram compact_repr
​af7f86c	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
​52d11e5	Added morphism to SymmetricGroupAlgebra to DiagramAlgebra and registered it as coercion
​08a04a7	Merge branch 'develop' into t/18720/change_diagram_algebra_basis_set_partitions_from_list_to_generator
​35b7663	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
​169e666	added missing import statement to diagram algebras file
​28ee71f	fixed the order of commands and import statements

[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?

[git]

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

[tscrim]

An MRO error is extremely strange. I will take a look at this as soon as I can.

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

[git]

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

​d16a7f5	tried moving SymmetricGroupAlgebra declaration in DiagramAlgebras init to no avail
​22c7116	temporary workaround for getting SymmetricGroupAlgebra initialized by initializing twice.

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

[git]

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

[git]

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.

[git]

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

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

[vdelecroix]

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

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

[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

[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

[git]

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

[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!

[tscrim]

Yes, these are also useful to verify that you are checking things properly. Also you should make sure to test corner cases.

[git]

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

[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?

[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::.

[git]

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

​a4c199d

added the necessary extra colon after tests

[tscrim]

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.

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

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