Skip to content
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

Refactor symmetric functions and k-bounded subspace #5457

Closed
nthiery opened this issue Mar 8, 2009 · 103 comments
Closed

Refactor symmetric functions and k-bounded subspace #5457

nthiery opened this issue Mar 8, 2009 · 103 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Mar 8, 2009

This patch restructures the implementation of symmetric functions in sage

The new implementation makes use of multiple realizations and the category framework. The new access to symmetric functions is via

sage: Sym = SymmetricFunctions(QQ)

Further new features that are implemented:

  • The ring of symmetric functions is now endowed with a Hopf algebra structure. The coproduct and antipode are implemented (which were missing before).

  • A tutorial on how to use symmetric functions in sage is included at the beginning of sf.py which is also accessible via

sage: SymmetricFunctions??
  • Symmetric functions should now work a lot better with respect to specializing parameters like q and t for Hall-Littlewood, Jack and Macdonald symmetric functions. Certain functionalities before this change were broken or not possible.

  • Documentation was added to LLT polynomials (which had very sparse documentation previously).

  • The k-bounded subspace of the ring of symmetric function was implemented. The k-Schur functions now live in the k-bounded subspace rather than in the ring of symmetric functions as before.

This patch gained tremendously by the tutorial on symmetric functions written by Jason Bandlow, a draft on the k-bounded subspace by Jason Bandlow, and code multiple realizations written by Franco Saliola.

See also http://groups.google.com/group/sage-devel/msg/a49f3288fca1b75c

Apply

Depends on #11563
Depends on #13109
Depends on #12969

CC: @sagetrac-sage-combinat @saliola @dwbump @sagetrac-chrisjamesberg @zabrocki @simon-king-jena

Component: combinatorics

Keywords: symmetric functions, days38, sd40

Author: Mike Zabrocki, Anne Schilling, Jason Bandlow

Reviewer: Dan Bump, Nicolas M. Thiéry, Jeroen Demeyer

Merged: sage-5.4.beta0

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

@nthiery

This comment has been minimized.

@anneschilling
Copy link

Dependencies: 13109

@anneschilling
Copy link

Changed dependencies from 13109 to none

@anneschilling
Copy link

Reviewer: Dan Bump, Franco Saliola

@anneschilling
Copy link

Author: Mike Zabrocki, Anne Schilling

@anneschilling

This comment has been minimized.

@anneschilling
Copy link

Changed keywords from none to symmetric functions, sd38, sd40

@anneschilling anneschilling changed the title Refactor symmetric functions Refactor symmetric functions and k-bounded subspace Jul 15, 2012
@anneschilling
Copy link

comment:10

Hi Mike,

I finished the doctests for the following files:

  • schur.py

  • homogeneous.py

  • elementrary.py

  • powersum.py

  • monomial.py

  • classical.py

  • dual.py

  • multiplicative.py

  • orthotriang.py

  • sf.py

In particular, at the beginning of sf.py I incorporated the tutorial that Jason and Nicolas
wrote (which was further down the queue) and updated it. I marked them there as coauthors in
that file.

This leaves the doctests for

  • hall_littlewood.py

  • jack.py

  • llt.py

  • macdonald.py

  • ns_macdonald.py

  • sfa.py

which I suppose you will do in the next couple of days?
In particular, in the sfa.py the deprecation warnings need to be activated which I have
not yet done.

Best,

Anne

@anneschilling
Copy link

Changed author from Mike Zabrocki, Anne Schilling to Mike Zabrocki, Anne Schilling, Jason Bandlow

@vbraun
Copy link
Member

vbraun commented Jul 16, 2012

Dependencies: #11563

@vbraun
Copy link
Member

vbraun commented Jul 16, 2012

Changed dependencies from #11563 to #11563, #13109

@anneschilling

This comment has been minimized.

@anneschilling
Copy link

comment:15

Hi Mike,

I completed the doctests for sfa.py and also rebased everything on top of 13109. Please put your changes to

  • hall_littlewood.py

  • jack.py

  • llt.py

  • macdonald.py

on top of the current patch trac_5457-symmetric_functions-mz.patch. Unfortunately we need to abandon the sage-combinat queue for the moment since it would be very cumbersome to keep it backward compatible with 13109. I will send you a separate e-mail on how to proceed.

Cheers,

Anne

@anneschilling
Copy link

comment:16

Ok, patch is ready for review! It should apply and run cleanly on sage.5.2.rc0!

Anne

@anneschilling
Copy link

@anneschilling
Copy link

comment:20

Hi Dan!

Thank you very much for your comments on the failing doctests in

  • devel/sage/sage/algebras/nil_coxeter_algebra.py

  • devel/sage/sage/categories/realizations.py

They are fixed in the updated version of the patch. I do not get failures for

  • devel/sage/sage/sandpiles/sandpile.py

on my machine.

lolita-4:sandpiles anne$ sage -t sandpile.py
sage -t "devel/sage-sf/sage/sandpiles/sandpile.py"
[19.0 s]


All tests passed!
Total time for all tests: 19.0 seconds

Anne

@dwbump
Copy link
Mannequin

dwbump mannequin commented Jul 24, 2012

comment:21

Replying to @anneschilling:

Hi Dan!

Thank you very much for your comments on the failing doctests in

  • devel/sage/sage/algebras/nil_coxeter_algebra.py

  • devel/sage/sage/categories/realizations.py

They are fixed in the updated version of the patch. I do not get failures for

  • devel/sage/sage/sandpiles/sandpile.py

on my machine.

I also get a doctest failure in sandpile.py with unpatched sage-5.2.rc0 so this failure is not caused by the patch.

@dwbump
Copy link
Mannequin

dwbump mannequin commented Jul 30, 2012

comment:22

Applies cleanly to sage-5.2 and passes all tests.

@anneschilling

This comment has been minimized.

@saliola
Copy link

saliola commented Sep 3, 2012

comment:89

Replying to @zabrocki:

Sorry I don't know what the [mq] is or how it got there.

The [mq] is short for mercurial queue; it is automatically placed there by hg.

@anneschilling
Copy link

comment:90

Hi Nicolas,

Technically, the Sage-Combinat queue can't handle patches that are merged and then unmerged. Now, this just means that we will have to everyone using a 5.3 beta to switch to 5.3 final; not a big deal.

I have no idea how to handle the sage-combinat queue now. Do nothing until sage-5.3 comes out and then force everyone to move to sage-5.3?

Anne, Mike: are there specific deadlines (e.g. the book or meetings) for which waiting for 5.4 could be a bother?

Beginning for the quarter for the book since my students are supposed to use it and sage!

Cheers,

Anne

@anneschilling
Copy link

comment:91

As far as I know this patch does not commute with #9265 which was just merged into sage. I hope Jeroen will rebase #5457 since I won't have time to do so!!!

Anne

@fchapoton
Copy link
Contributor

comment:92

Apply trac_5457-symmetric_functions-mz.patch, 5457_long_time.patch, trac_5457_llt_doc_and_bug_fix-mz.2.patch

@anneschilling
Copy link

Changed reviewer from Dan Bump, Nicolas M. Thiéry to Dan Bump, Nicolas M. Thiéry, Jeroen Demeyer

@anneschilling
Copy link

Changed author from Mike Zabrocki, Anne Schilling, Jason Bandlow, Jeroen Demeyer to Mike Zabrocki, Anne Schilling, Jason Bandlow

@jdemeyer
Copy link

jdemeyer commented Sep 4, 2012

Merged: sage-5.4.beta0

@jdemeyer
Copy link

Second additional patch

@jdemeyer
Copy link

comment:95

Attachment: 5457_long_time_2.patch.gz

My second additional patch attachment: 5457_long_time_2.patch needs review (just mention the review in the comments, don't change the status).

@jdemeyer

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented Sep 21, 2012

comment:96

Replying to @jdemeyer:

My second additional patch attachment: 5457_long_time_2.patch needs review (just mention the review in the comments, don't change the status).

Positive review! Thanks!

@jdemeyer
Copy link

comment:97

This patch abuses assert and AssertionError. assert should not be used for control flow. An assert checks something which should always be true, a failed assertion is always a bug in the program.

For example:

            sage: f.skew_by([1])
            Traceback (most recent call last):
            ...
            AssertionError: x needs to be a symmetric function

This is a simple user mistake, for which assert is not right.

I think this must be fixed.

@nthiery
Copy link
Contributor Author

nthiery commented Sep 24, 2012

comment:98

Hi Jeroen,

Replying to @jdemeyer:

This patch abuses assert and AssertionError. assert should not be used for control flow. An assert checks something which should always be true, a failed assertion is always a bug in the program.

For example:

            sage: f.skew_by([1])
            Traceback (most recent call last):
            ...
            AssertionError: x needs to be a symmetric function

This is a simple user mistake, for which assert is not right.

I think this must be fixed

Given the discussion on sage-devel, do we agree that there is no control flow involved and it's a not so common function, so it's ok to use assert?

Cheers,
Nicolas

@anneschilling
Copy link

comment:99

Replying to @jdemeyer:

This patch abuses assert and AssertionError. assert should not be used for control flow. An assert checks something which should always be true, a failed assertion is always a bug in the program.

For example:

            sage: f.skew_by([1])
            Traceback (most recent call last):
            ...
            AssertionError: x needs to be a symmetric function

This is a simple user mistake, for which assert is not right.

I think this must be fixed.

We had a user who used this method wrongly at FPSAC (he used a partition instead of a symmetric function). That's why we added this (since this can potentially be a common user mistake). If this should be done differently, feel free to change it!

Anne

@jdemeyer
Copy link

comment:100

Replying to @anneschilling:

That's why we added this (since this can potentially be a common user mistake). If this should be done differently, feel free to change it!

Yes, it should be done differently. The correct way would be:

if not needed_condition:
    raise ValueError("Error message")

As I said: assert is only meant to catch bugs, not user errors. Although, as Nicolas Thiéry argued, something assert is acceptible. See also the sage-devel thread.

@anneschilling

This comment has been minimized.

@anneschilling
Copy link

comment:101

Attachment: trac_5457-symmetric_functions-mz.2.patch.gz

@anneschilling
Copy link

comment:102

I folded the patches and made the required change.

Apply: trac_5457-symmetric_functions-mz.2.patch

Anne

@nthiery
Copy link
Contributor Author

nthiery commented Sep 26, 2012

comment:103

Hi Jeroen,

I am confused: was your intention to request that this change be made for this ticket?

(meaning that you'd have to unmerge / remerge it)

@jdemeyer
Copy link

comment:104

Short story: for me it was far more important that the problem got
fixed, not how it got fixed.

A situation where a ticket is merged but then problems are discovered is
a difficult situation to handle. In the past, I would have re-opened
the ticket and set it to needs_work but that seems to upset people (they
think it will postpone the merging of their patch). So this time, I
decided not to reopen the ticket but my intention was indeed for the
chances to be made on the same ticket. However, ideally it would have
been done in a separate additional patch, leaving the original
already-merged patch alone. But I certainly would have been happy also
with a new ticket.

I have no idea how any of this affect sage-combinat.

Jeroen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants