-
-
Notifications
You must be signed in to change notification settings - Fork 567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement NCSym #15150
Comments
comment:1
[Answered my own question.] |
comment:2
Here's what I currently have. The multiplication in the dual basis isn't working correctly yet. I know there's the |
comment:3
The only problem is that I don't have much time these days to work on this. I've been meaning to implement NCSym. I'll try to take a look at what you have and see what I can offer, if anything. As usual, thank you for your work, Travis! |
comment:4
Travis, what sources are you using for this? I'm asking because I haven't read anything about NCSym so far. I have a vague impression it has been introduced in one of Rota's Foundations papers, but not one that I have. A good introduction would simplify my job reviewing this by a lot. |
comment:5
Here's one of the references (Bergeron, Zabrocki): http://arxiv.org/pdf/math/0509265.pdf. I forget the other references off-hand (it's on my other computer tho), and I'll be adding all of the references to the doc as I finish it up. |
This comment has been minimized.
This comment has been minimized.
comment:6
Also I just fixed the w basis multiplication. |
comment:7
Okay, the patch is now ready for first review. It has full doctest coverage and all tests pass for me. I've removed some of the coercions in the old version since I was worried about possibly breaking coercion commutative diagrams. Some of the things I'm not perfectly happy about (but can live with currently):
The references I used are included in the documentation now too. |
comment:8
Hi Travis, One initial comment about the |
comment:9
New version rebased over changes in #15143 and removes the word "natural" and instead states that it is the map which fixes |
comment:10
Hi Travis, What patches and version do you have applied? I get fuzz on the file sage/combinat/ncsf_qsym/ncsf.py and doc tests failing in ncsym.py (seemingly) because the output displays in a different order. We may need to put a dependency with other patches that touch ncsf.py. I am currently running on sage-5.12.beta5 and only #15143 applied. |
comment:11
Hey Mike, I have 5.12.beta3 in the combinat queue, but the fuzz is likely due to #15164 (which I'm planning to review soon). What type of fuzz do you get? If it's 0 or 1, we don't need to put it down as a dependency. I'll build 5.12.beta5 today and see if I also get doctest failing. Best, Travis |
comment:12
You are right that the fuzz is disappears by applying #15164 I get 4 doctest failures on ncsym.py. They are minor, but I don't know why I see them and you don't (so it doesn't seem like a good idea to change them). They are on lines 248, 438, 633, 737 and for instance one is
|
comment:13
I would have thought that the total ordering on set partitions in #15143 would have
My guess as to why we're getting different output is a failure in the comparisons (I didn't see any tickets that seemed like would change the output of
Do you get anything different, in particular the last one? |
comment:14
I do get something different, but all of the comparisons are the same. For the last one I get: Any idea why? |
comment:15
This is why:
The I'm fixing this right now. |
comment:16
Okay, now it should be consistent.
I added a |
comment:17
Attachment: trac_15150_product_on_basis_q-mz.patch.gz OK. Thanks for fixing that. I'm looking over the patch and I have some initial changes where the multiplication uses the pipe function and the product is implemented in the The change of basis from I think that line 180 from DNCSym is similar to quasi-symmetric functions so that if you add the right elements together (say some linear combination of the |
comment:18
Another thing that was bothering me was that |
comment:19
Some random comments on ncsym.py.
If the variables are non-commuting, use angular rather than square brackets. Also, I assume you want infinitely many variables? And do you really want compositions? Typo:
Old-style arXiv references like this:
should have a "math/" in front of them, I believe (also, I'd add a version number, in this case math/0208168v2). What is the \wedge operation on set partitions? It is used but not defined. If it's the one from the Bergeron-Zabrocki paper, it is simply the meet of set partitions (aka On the right hand side of
don't you mean to say Similarly here:
I assume that "strictly coarser" in
refers to the relation of strict coarsening as defined in I'll eventually have a closer look at the patch if only to understand how exactly internal-coproduct-by-coercion works (for use in NSym); I cannot promise that I will ever give this an actual review. Nevertheless, great work here, Travis. |
comment:20
Here's the new version of the patch which (hopefully) takes care of all of Darij's comments. How the I've removed the coercion from Currently to get from For Thank you all for looking at this patch, Travis Edit - let's see if the patchbot catches it in an edited message: For patchbot: Apply: trac_15150-ncsym-ts.patch |
This comment has been minimized.
This comment has been minimized.
comment:21
Hi Travis, |
comment:22
I think I uploaded the patch from my beta3 install instead of my beta5. Sorry about that. Here's the new one. For patchbot: Apply: trac_15150-ncsym-ts.patch |
comment:76
I am not comfortable making That is, let phi : NCSym -> Sym That is, "equalities go to equalities, but sometimes inequalities also go to equalities." (to my eye, not something I want as a coercion). This isn't a hard fast rule, but it seems counter-intuitive to be able to coerce from NCSym to Sym rather than use the method For psi: Sym -> NCSym* we have psi(F)=psi(G) iff F=G, that is, "equalities going to equalities and inequalities going to inequalities" |
comment:77
Since it's consistent with |
comment:78
I want to be completely convinced everything works properly and haven't missed any details in the documentation. Give me another day or two to find minor tweaks. Otherwise I am extremely happy with it. Nantel asked just today to use it to compute |
comment:79
current version moves |
comment:80
Can you check if |
comment:81
Attachment: trac_15150-docchanges-mz.patch.gz My tests on the antipode seems to run pretty fast without caching, but please take a look. I just added a new version with some minor doc corrections. |
comment:82
All tests pass, and documentation looks good. please look over my last version of the patch, fold it with yours, make any last changes, and you have my blessing on a positive review. Nice work! |
comment:83
A couple of things left to change in the version you posted. Search on SEEALSO because it seems to be duplicated in three places. Also I think that occurrences of |
comment:84
Good catch; merge issue. Fixed and same with the map composition order. Since #10963 is finished (yay), I've added it as a dependency since it seems to affect the output of one of the doctests in Best, Travis For patchbot: Apply: trac_15150-ncsym-ts.patch |
Reviewer: Mike Zabrocki, Darij Grinberg |
comment:85
Okay, now everything should be squared away wrt the dep on #10963. For patchbot: Apply: trac_15150-ncsym-ts.patch |
comment:86
I used sage cloud and git to test this patch. All tests pass (even with #10963 applied). I think that we can be confident that once it has a positive review, that this will pass as well. |
comment:88
yay milestone tug of war! Is there an easy way to commute this past #10963 or will that make code slower/buggier/whatever? |
Attachment: trac_15150-ncsym-ts.patch.gz with fixes |
comment:89
Since #10963 is getting pushed back, I've changed the one doctest output to move this past. For patchbot: Apply: trac_15150-ncsym-ts.patch |
comment:90
I cannot promise this will be merged in Sage 5.13 though. Did you test this well, that will increase the chances? |
comment:91
All tests on the file passed for me on |
comment:92
Works for me on
And the pdf documentation builds cleanly:
|
Merged: sage-5.13.rc0 |
Implement the Hopf algebra of symmetric functions in non-commuting variables in the following bases:
as well as the dual basis w.
Apply: attachment: trac_15150-ncsym-ts.patch
Depends on #15143
Depends on #15164
CC: @sagetrac-sage-combinat @zabrocki @saliola @darijgr
Component: combinatorics
Author: Travis Scrimshaw
Reviewer: Mike Zabrocki, Darij Grinberg
Merged: sage-5.13.rc0
Issue created by migration from https://trac.sagemath.org/ticket/15150
The text was updated successfully, but these errors were encountered: