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

Make lrcalc a standard package #11563

Closed
nthiery opened this issue Jul 1, 2011 · 51 comments
Closed

Make lrcalc a standard package #11563

nthiery opened this issue Jul 1, 2011 · 51 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Jul 1, 2011

#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 attachment: lrcalc-1.1.6beta1.spkg as standard spkg.
  2. apply attachment: trac_11563-lrcalc_standard_package-nt.patch to the Sage library.
  3. apply attachment: trac_11563-lrcalc-sageroot-nt.patch to SAGE_ROOT.

Depends on #10333

CC: @sagetrac-sage-combinat @kiwifb @asbuch @saliola

Component: combinatorics

Keywords: lrcalc, symmetric functions, days38

Author: Nicolas M. Thiéry

Reviewer: Anne Schilling, Jeroen Demeyer, John Palmieri

Merged: sage-5.2.rc0

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

@nthiery
Copy link
Contributor Author

nthiery commented May 10, 2012

Author: Nicolas M. Thiéry

@nthiery
Copy link
Contributor Author

nthiery commented May 10, 2012

Reviewer: Anne Schilling

@nthiery
Copy link
Contributor Author

nthiery commented May 10, 2012

Changed keywords from none to lrcalc, symmetric functions

@nthiery

This comment has been minimized.

@nthiery nthiery modified the milestones: sage-5.0, sage-5.1 May 10, 2012
@anneschilling
Copy link

Changed keywords from lrcalc, symmetric functions to lrcalc, symmetric functions, days38

@anneschilling

This comment has been minimized.

@anneschilling
Copy link

comment:5

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

Anne

@nthiery
Copy link
Contributor Author

nthiery commented May 10, 2012

comment:6

Thanks Anne!

Same comment as Mike :-)

@jdemeyer
Copy link

comment:7

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

@nthiery
Copy link
Contributor Author

nthiery commented May 11, 2012

comment:8

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

@anneschilling
Copy link

comment:9

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

@jdemeyer
Copy link

comment:10

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

@jdemeyer
Copy link

Changed reviewer from Anne Schilling to Anne Schilling, Jeroen Demeyer

@jdemeyer

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented May 19, 2012

comment:12

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

@nthiery

This comment has been minimized.

@jdemeyer
Copy link

comment:32

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

@jdemeyer
Copy link

Changed merged from sage-5.2.beta1 to none

@jdemeyer jdemeyer reopened this Jul 12, 2012
@anneschilling
Copy link

comment:33

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

Thanks,

Anne

@jhpalmieri

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented Jul 13, 2012

comment:35

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

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented Jul 13, 2012

comment:38

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

@jhpalmieri
Copy link
Member

comment:39

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.

@nthiery
Copy link
Contributor Author

nthiery commented Jul 13, 2012

Attachment: lrcalc-1.1.6beta1.spkg.gz

@nthiery
Copy link
Contributor Author

nthiery commented Jul 13, 2012

Diff to previous positive reviewed spkg

@nthiery
Copy link
Contributor Author

nthiery commented Jul 13, 2012

comment:40

Attachment: lrcalc-1.1.6beta1.spkg.diff.gz

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

@jhpalmieri
Copy link
Member

comment:41

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

@anneschilling
Copy link

comment:42

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!

@anneschilling
Copy link

Changed reviewer from Anne Schilling, Jeroen Demeyer to Anne Schilling, Jeroen Demeyer, John Palmieri

@jdemeyer
Copy link

Merged: sage-5.2.rc0

@pcpa
Copy link
Mannequin

pcpa mannequin commented Sep 7, 2012

comment:45

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.

@jdemeyer
Copy link

jdemeyer commented Aug 4, 2014

comment:46

Please review the trivial follow-up ticket #16756.

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

4 participants