Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#11563 closed enhancement (fixed)

Make lrcalc a standard package

Reported by: nthiery Owned by: sage-combinat
Priority: major Milestone: sage-5.2
Component: combinatorics Keywords: lrcalc, symmetric functions, days38
Cc: sage-combinat, fbissey, asbuch, saliola Merged in: sage-5.2.rc0
Authors: Nicolas M. Thiéry Reviewers: Anne Schilling, Jeroen Demeyer, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #10333 Stopgaps:

Description (last modified by nthiery)

#10333 implements an interface to lrcalc, using an optional spkg. According to the vote on http://groups.google.com/group/sage-devel/browse_thread/thread/2e7114375f6f88a5/, there is a consensus on making lrcalc a standard spkg, after a one release probation period in order to follow the official rule.

  1. spkg: add lrcalc-1.1.6beta1.spkg as standard spkg.
  2. apply trac_11563-lrcalc_standard_package-nt.patch to the Sage library.
  3. apply trac_11563-lrcalc-sageroot-nt.patch to SAGE_ROOT.

Attachments (4)

trac_11563-lrcalc_standard_package-nt.patch (13.2 KB) - added by nthiery 5 years ago.
trac_11563-lrcalc-sageroot-nt.patch (1.3 KB) - added by jdemeyer 5 years ago.
lrcalc-1.1.6beta1.spkg (614.5 KB) - added by nthiery 5 years ago.
lrcalc-1.1.6beta1.spkg.diff (3.2 KB) - added by nthiery 5 years ago.
Diff to previous positive reviewed spkg

Download all attachments as: .zip

Change History (50)

comment:1 Changed 6 years ago by fbissey

  • Cc fbissey added

comment:2 Changed 5 years ago by nthiery

  • Authors set to Nicolas M. Thiéry
  • Description modified (diff)
  • Keywords lrcalc symmetric functions added
  • Milestone changed from sage-5.0 to sage-5.1
  • Reviewers set to Anne Schilling
  • Status changed from new to needs_review

comment:3 Changed 5 years ago by aschilling

  • Keywords days38 added

comment:4 Changed 5 years ago by aschilling

  • Description modified (diff)

comment:5 Changed 5 years ago by aschilling

  • Status changed from needs_review to positive_review

I ran the tests and looked at the documentation. Everything looks fine.

Anne

comment:6 Changed 5 years ago by nthiery

Thanks Anne!

Same comment as Mike :-)

comment:7 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This needs patches to spkg/install and spkg/standard/deps.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by nthiery

Hi Jeroen,

Replying to jdemeyer:

This needs patches to spkg/install and spkg/standard/deps.

Ah sorry, I missed that (that's my first standard spkg). Do you mind reviewing my changes? You probably know better than anyone about them :-)

Thanks in advance!

Cheers,

Nicolas

comment:9 in reply to: ↑ 8 Changed 5 years ago by aschilling

Replying to nthiery:

Hi Jeroen,

Replying to jdemeyer:

This needs patches to spkg/install and spkg/standard/deps.

Ah sorry, I missed that (that's my first standard spkg). Do you mind reviewing my changes? You probably know better than anyone about them :-)

Thanks in advance!

Cheers,

Nicolas

What is the status on this? It would be good to get this patch into sage soon so we can build on it (and avoid using symmetrica).

Thanks,

Anne

comment:10 follow-up: Changed 5 years ago by jdemeyer

You should use $MAKE instead of make in spkg-install and spkg-check.

Some of the files in the spkg are not world-readable. Please fix this with

chmod ugo+rX -R src

The SAGE_ROOT patch trac_11563-lrcalc-sageroot-nt.patch gets positive_review from me.

Finally: adding a new standard package should always be discussed on the sage-devel mailing list.

comment:11 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers changed from Anne Schilling to Anne Schilling, Jeroen Demeyer

comment:12 in reply to: ↑ 10 Changed 5 years ago by nthiery

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

You should use $MAKE instead of make in spkg-install and spkg-check.

Some of the files in the spkg are not world-readable. Please fix this with

chmod ugo+rX -R src

Thanks for catching those. Done. Anne: may I let you set the final positive review? Here is the diff in the updated version I am about to upload (sorry, it does not show up permission changes):

diff --git a/spkg-check b/spkg-check
--- a/spkg-check
+++ b/spkg-check
@@ -8,7 +8,7 @@ fi
 
 cd src
 
-make check
+$MAKE check
 if [ $? -ne 0 ]; then
    echo "Error testing lrcalc."
    exit 1
diff --git a/spkg-install b/spkg-install
--- a/spkg-install
+++ b/spkg-install
@@ -14,13 +14,13 @@ if [ $? -ne 0 ]; then
    exit 1
 fi
 
-make
+$MAKE
 if [ $? -ne 0 ]; then
    echo "Error building lrcalc."
    exit 1
 fi
 
-make install
+$MAKE install
 if [ $? -ne 0 ]; then
    echo "Error installing lrcalc."
    exit 1

The SAGE_ROOT patch trac_11563-lrcalc-sageroot-nt.patch gets positive_review from me.

Thanks.

Finally: adding a new standard package should always be discussed on the sage-devel mailing list.

We did it a long time ago :-)

http://groups.google.com/group/sage-devel/browse_thread/thread/2e7114375f6f88a5/ca4e2ecb49fcc1cd?lnk=gst&q=lrcalc#ca4e2ecb49fcc1cd

Cheers,

Nicolas

comment:13 follow-up: Changed 5 years ago by nthiery

  • Description modified (diff)

comment:14 in reply to: ↑ 13 Changed 5 years ago by aschilling

Replying to nthiery:

Looks good to me if Jeroen is happy!

Anne

comment:15 Changed 5 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:16 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from positive_review to needs_work

When you change a package, you should change the version number (say, to lrcalc-1.1.6beta.p0.spkg) and change SPKG.txt accordingly.

comment:17 in reply to: ↑ 16 Changed 5 years ago by nthiery

Replying to jdemeyer:

When you change a package, you should change the version number (say, to lrcalc-1.1.6beta.p0.spkg) and change SPKG.txt accordingly.

Ok, there it is ...

comment:18 Changed 5 years ago by nthiery

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:19 follow-up: Changed 5 years ago by jhpalmieri

  • Description modified (diff)

I think that in SPKG.txt, at the top it should just say "lrcalc" rather than "lrcalc 1.1.6 beta p0": the version number should just be in the ChangeLog (for ease of updating later). I took the liberty of creating a new spkg (committed in your name, Nicolas). I'm also attaching a patch showing the changes.

comment:20 in reply to: ↑ 19 Changed 5 years ago by nthiery

Replying to jhpalmieri:

I think that in SPKG.txt, at the top it should just say "lrcalc" rather than "lrcalc 1.1.6 beta p0": the version number should just be in the ChangeLog (for ease of updating later). I took the liberty of creating a new spkg (committed in your name, Nicolas). I'm also attaching a patch showing the changes.

All good for me. Thanks.

comment:21 Changed 5 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:22 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:23 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:24 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:25 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This needs to be rebased to sage-5.1.beta6:

patching file doc/en/reference/libs.rst
Hunk #1 FAILED at 26.
1 out of 1 hunk FAILED -- saving rejects to file doc/en/reference/libs.rst.rej
patching file sage/libs/lrcalc/lrcalc.pyx

comment:26 in reply to: ↑ 25 ; follow-up: Changed 5 years ago by nthiery

Replying to jdemeyer:

This needs to be rebased to sage-5.1.beta6:

patching file doc/en/reference/libs.rst
Hunk #1 FAILED at 26.
1 out of 1 hunk FAILED -- saving rejects to file doc/en/reference/libs.rst.rej
patching file sage/libs/lrcalc/lrcalc.pyx

Arr, what a pain; we are being soooo inefficient on this one.

Ok, I am downloading 5.1.rc0, and rebase this patch this afternoon or so. It would be good to have this spkg in 5.1 for Sage Days 40!

Changed 5 years ago by nthiery

comment:27 in reply to: ↑ 26 Changed 5 years ago by nthiery

  • Status changed from needs_work to needs_review

Replying to nthiery:

Arr, what a pain; we are being soooo inefficient on this one.

Ok, I am downloading 5.1.rc0, and rebase this patch this afternoon or so. It would be good to have this spkg in 5.1 for Sage Days 40!

Ok, I just updated the patch, rebased on 5.1.rc0. The only change I had to do was:

  • doc/en/reference/libs.rst

    diff --git a/doc/en/reference/libs.rst b/doc/en/reference/libs.rst
    a b to be aware of the modules described in  
    2727   
    2828   sage/libs/flint/fmpz_poly
    2929   sage/libs/libecm
     30   sage/libs/lrcalc/lrcalc
    3031   sage/libs/pari/gen
    3132   sage/libs/ntl/all
    3233   sage/libs/mwrank/all

Back to needs review ...

Jeroen: if you feel like it, feel free to do such trivial rebases directly; it probably will take you less time than going through this whole process.

Cheers,

Nicolas

comment:28 Changed 5 years ago by aschilling

  • Status changed from needs_review to positive_review

Changed 5 years ago by jdemeyer

comment:29 follow-up: Changed 5 years ago by jdemeyer

SAGE_ROOT patch rebased to the upcoming sage-5.2.beta0.

comment:30 in reply to: ↑ 29 Changed 5 years ago by nthiery

Replying to jdemeyer:

SAGE_ROOT patch rebased to the upcoming sage-5.2.beta0.

Thanks!

comment:31 Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.2.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:32 follow-up: Changed 5 years ago by jdemeyer

  • Merged in sage-5.2.beta1 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

lrcalc fails on hawk (OpenSolaris? 06.2009-32):

buildbot@hawk:~/sage-5.2.beta0$ ./sage --gdb
----------------------------------------------------------------------
| Sage Version 5.2.beta0, Release Date: 2012-07-09                   |
| Type "notebook()" for the browser-based notebook interface.        |
| Type "help()" for help.                                            |
----------------------------------------------------------------------
**********************************************************************
*                                                                    *
* Warning: this is a prerelease version, and it may be unstable.     *
*                                                                    *
**********************************************************************
/export/home/buildbot/sage-5.2.beta0/local/bin/sage-ipython
GNU gdb 6.8
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i386-pc-solaris2.11"...
warning: Lowest section in /lib/libdl.so.1 is .dynamic at 00000074
Python 2.7.3 (default, Jul 11 2012, 15:59:32)
[GCC 4.4.3 20100112 (prerelease)] on sunos5
Type "help", "copyright", "credits" or "license" for more information.
warning: Lowest section in /lib/libintl.so.1 is .dynamic at 00000074
warning: Lowest section in /lib/libpthread.so.1 is .dynamic at 00000074
sage: from sage.libs.lrcalc import lrcalc
sage: lrcalc.mult([Integer(3),Integer(2),Integer(1)], [Integer(3),Integer(2),Integer(1)], Integer(3),Integer(2))

Program received signal SIGSEGV, Segmentation fault.
0x00000064 in ?? ()
(gdb) bt
#0  0x00000064 in ?? ()
#1  0xfd699fb1 in hash_insert (h=0xc107940, k=0xc108c58, v=0x1) at ../src/language/hash.c:52
#2  0xf6504bac in fusion_reduce (lc=0xc107940, rows=3, cols=2, opt_zero=0) at symfcn.c:866
#3  0xf6529a7f in __pyx_pf_4sage_4libs_6lrcalc_6lrcalc_4mult (__pyx_self=0x0, __pyx_args=0x8102374, __pyx_kwds=0x0)
    at sage/libs/lrcalc/lrcalc.c:2370
#4  0xfee7d7a0 in PyCFunction_Call (func=0xc0de02c, arg=0x8102374, kw=0xc108c58) at Objects/methodobject.c:85
#5  0xfeed917b in PyEval_EvalFrameEx (f=0xc1372ec, throwflag=0) at Python/ceval.c:4021
#6  0xfeed9c8b in PyEval_EvalCodeEx (co=0xc0da020, globals=0x851eb54, locals=0x851eb54, args=0x0, argcount=0, kws=0x0, kwcount=0,
    defs=0x0, defcount=0, closure=0x0) at Python/ceval.c:3253
[...]

comment:33 in reply to: ↑ 32 Changed 5 years ago by aschilling

Could Nicolas and I get an account on hawk to investigate this? Or will you?

Thanks,

Anne

comment:34 Changed 5 years ago by jhpalmieri

  • Description modified (diff)

comment:35 Changed 5 years ago by nthiery

  • Description modified (diff)
  • Status changed from new to needs_review

Thank to John, I could login on hawk and investigate. It turned out to be a C-level name conflict with a function called hash_insert. I used a macro to rename that function in the lrcalc sources.

I also used the occasion to add examples of fusion and quantum calculations in the README and testsuite.

I bumped lrcalc's version accordingly. Anders: please finish up version 1.1.6 from there!

The updated spkg also only installs the lrcalc libraries and headers, not the binaries. Thanks John for pointing this out!

See the attached diff for the changes.

Quick review would be most appreciated to get this in 5.2!!!

Thanks,

Nicolas

comment:36 Changed 5 years ago by nthiery

  • Cc asbuch saliola added

comment:37 Changed 5 years ago by nthiery

  • Description modified (diff)

comment:38 Changed 5 years ago by nthiery

I should mention: installation and tests passed smoothly on hawk and a ubuntu machine.

comment:39 follow-up: Changed 5 years ago by jhpalmieri

Since the binaries are no longer installed, the test suite for the package fails (that is, running sage -i -c lrcalc...):

Successfully installed lrcalc-1.1.6beta1
Running the test suite for lrcalc-1.1.6beta1...
Making check in mathlib
make[1]: warning: -jN forced in submake: disabling jobserver mode.
make[1]: Nothing to be done for `check'.
Making check in lrcoef
make[1]: warning: -jN forced in submake: disabling jobserver mode.
make[1]: Nothing to be done for `check'.
Making check in schubert
make[1]: warning: -jN forced in submake: disabling jobserver mode.
make[1]: Nothing to be done for `check'.
make[1]: warning: -jN forced in submake: disabling jobserver mode.
make -j2  check-TESTS
make[2]: warning: -jN forced in submake: disabling jobserver mode.
-n testing lrcoef/lrcoef 3 2 1 - 2 1 - 2 1  ... 
ok
-n testing lrcoef/skew 3 2 1 - 2 1          ... 
ok
-n testing lrcoef/skew -r 2 3 2 1 / 2 1     ... 
ok
-n testing lrcoef/mult 2 1 - 2 1            ... 
ok
./testsuite: line 7: mult: command not found
-n testing mult -f 3,2 3 2 1 - 3 2 1        ... 
failed:
  Expected: 1  (4, 4, 4)
1  (5, 4, 3)
  Got: 
./testsuite: line 7: mult: command not found
-n testing mult -q 3,2 3 2 1 - 3 2 1        ... 
failed:
  Expected: 1  (2)
1  (1, 1)
  Got: 
-n testing lrcoef/coprod 3 2 1              ... 
ok
-n testing schubert/schubmult 1 3 2 - 1 3 2 ... 
ok
FAIL: testsuite
=======================================================
1 of 1 test failed
Please report to asbuch at math dot rutgers period edu 
=======================================================

Otherwise, all tests passed for me on hawk also, and on OS X 10.7. I think that since this had a positive review before, if you can fix spkg-check so self-tests pass, then it should get a positive review.

Changed 5 years ago by nthiery

Changed 5 years ago by nthiery

Diff to previous positive reviewed spkg

comment:40 in reply to: ↑ 39 Changed 5 years ago by nthiery

Replying to jhpalmieri:

Since the binaries are no longer installed, the test suite for the package fails

Oops, fixed (I was calling mult incorrectly in the new tests). Thanks for catching this! Updated spkg and diff attached.

Thanks for the super quick review :-)

Cheers,

Nicolas

comment:41 follow-up: Changed 5 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

Looks good now. Passes its test suite and lrcalc.pyx passes doctests on sage.math, hawk, and OS X 10.7.

comment:42 in reply to: ↑ 41 Changed 5 years ago by aschilling

Replying to jhpalmieri:

Looks good now. Passes its test suite and lrcalc.pyx passes doctests on sage.math, hawk, and OS X 10.7.

Thanks, John, for running the final tests and giving access to hawk! You should add yourself to the list of reviewers!

comment:43 Changed 5 years ago by aschilling

  • Reviewers changed from Anne Schilling, Jeroen Demeyer to Anne Schilling, Jeroen Demeyer, John Palmieri

comment:44 Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.2.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:45 Changed 5 years ago by pcpa

Is there some prevision of an official release of lrcalc 1.1.6? I would like to package it in fedora, and believe the sagemath spkg is not mean't to be the upstream tarball. It should be good enough if upstream can make the 1.1.6beta tarball available. But it has plenty of time as I deferred my fedora sagemath package to after fedora 18.

comment:46 Changed 3 years ago by jdemeyer

Please review the trivial follow-up ticket #16756.

Note: See TracTickets for help on using tickets.