Make lcalc compatible with the new PARI #11321

jdemeyer · 2011-05-10T15:28:26Z

PARI version 2.4.4 (see #11130, now aiming at 2.5.0) no longer has an allocatemoremem() function, which lcalc uses. There is a function allocatemem() which can be used instead.

The second reviewer patch (not in the spkg) removes the requirement for a PARI >= 2.4.4 spkg, i.e. with that applied it will build with our current ones as well.

New spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-1.23.p9.spkg (this will only build after installing the PARI spkg from #11130).

Reviewers should look at attachment: lcalc-1.23.p6-p9.diff to see the difference with the lcalc spkg currently in Sage.

Depends on #11130

Upstream: Reported upstream. Developers acknowledge bug.

CC: @kiwifb @rishikesha

Component: packages: standard

Keywords: lcalc spkg

Author: Jeroen Demeyer, Leif Leonhardy

Reviewer: Volker Braun, Leif Leonhardy, William Stein

Merged: sage-4.8.alpha1

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

The text was updated successfully, but these errors were encountered:

vbraun · 2011-05-10T16:12:05Z

Reviewer: Volker Braun

vbraun · 2011-05-10T16:12:05Z

comment:1

Looks good to me. Are you making a full lcalc spkg at one point?

jdemeyer · 2011-05-11T07:18:57Z

comment:3

Replying to @vbraun:

Looks good to me. Are you making a full lcalc spkg at one point?

Yes, when I have time.

jdemeyer · 2011-05-12T15:51:41Z

Dependencies: #11130

jdemeyer · 2011-05-12T15:52:24Z

Upstream: Reported upstream. Little or no feedback.

jdemeyer · 2011-05-12T15:55:02Z

comment:7

New spkg ready for review, needs to be built with the new PARI spkg.

jdemeyer · 2011-05-12T18:10:21Z

Changed upstream from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.

jdemeyer · 2011-05-12T18:10:21Z

comment:8

On 2011-05-12 18:56, Michael Rubinstein wrote:

Thanks for the patch.

I should point out that I now have a google code page for lcalc:

http://code.google.com/p/l-calc/

(lcalc was taken, so I went with l-calc).

The page has my beta newer version of lcalc, L-1.3
However, Rishikesh's cython wrapper is still being updated for that, so it
won't work within sage with the current wrapper.

I should point out that Lcommandline_elliptic.cc has become part of
the underlying class library and moved to
Lelliptic.cc in version L-1.3

Mike

nexttime · 2011-07-17T17:42:34Z

comment:12

The patch to the Makefile still uses -lmpir instead of -lgmp. (We always build MPIR with GMP compatibility, as all other packages refer to GMP rather than MPIR.) And the receipt for all is still funny, but that's upstream...

I'd quote $patch to be on the safe side. Also, the patches are / have to be applied with -p1 (from lcalc-*/src/) rather than -p0, which I don't mind, but Jeroen wanted to unify this across spkgs IIRC.

nexttime · 2011-07-17T19:46:59Z

comment:13

spkg-install:

The comment and a message regarding SAGE_DEBUG is not consistent. (SAGE_DEBUG has to be set to yes to enable debugging support, while anything else disables it, though not deleting any user-specified flags of course.) I'd always add -g anyway, since this doesn't have any measureable performance impact, only the object files will grow slightly.
$CC must not be quoted in the compiler test (if we support flags within CC rather than compiler paths containing spaces). Perhaps a matter of taste, but this should be consistent in Sage, and e.g. distutils uses the former variant, not to mention the way we use $MAKE. Same applies to $CXX and in principle $SAGE_FORTRAN, while the Fortran compiler test is useless there anyway.
Funny that Dave uses test -e for the Fortran library...
I wonder if error messages etc. should be redirected to stderr as we started to do in a couple of scripts. The disadvantage of doing so is that both streams occasionally get out of sync due to buffering, leading to potentially confusing output.
Copy sage specific modifications should now of course read Apply Sage ....
$UNAME should also be quoted (twice); case ... in ... would anyway be better.
I'm not sure if Cygwin needs both a .dll and a .so file (cf. Copy needed dll to so files on Cygwin #11547). Currently the .dll is copied to an .so in $SAGE_LOCAL/lib/. Unless it is statically linked, the stand-alone lcalc might also require the shared library in $SAGE_LOCAL/bin/.

SPKG.txt:

The Upstream contact section should be moved up.
Special Build Instructions should read ... Update/Build ....
The Changelog heading is missing.
The Dependencies section is completely missing.

Some files from src/include/ can also be deleted (and I doubt we have to copy all of them to $SAGE_LOCAL/include/lcalc/; L*.h and mpfr_mul_d.h -- included by Lgmpfrxx.h -- should suffice):

~/Sage/spkgs/lcalc-1.23.p7$ ls src/include/
cmdline.h                      Lfind_zeros.h
getopt.h                       Lgamma.h
Lcommandline_elliptic.h        Lglobals.h
Lcommandline_globals.h         Lgmpfrxx.h
Lcommandline.h                 Lgram.h
Lcommandline_misc.h            L.h
Lcommandline_numbertheory.h    Lint_complex.h
Lcommandline_twist.h           Lmisc.h
Lcommandline_values_zeros.h    Lnumberzeros.h
Lcommon.h                      Lnumeric.h
Lcommon_ld.h                   Lprint.h
Lcomplex.h                     Lriemannsiegel_blfi.h
Ldirichlet_series.h            Lriemannsiegel.h
Ldokchitser.h                  Lvalue.h
Lexplicit_formula.h            Lvalue.h.bak
Lexplicit_formula.h.swap.crap  mpfr_mul_d.h

I wonder if it's worth to run Lcalc's tests from an spkg-check file.

I'd really appreciate having the changes to spkgs to be "finally" reviewed committed, especially if files get deleted, as is the case here. Also, IMHO commit messages are subject to review as well, no matter if some merge script would add a generic one in case it is missing.

nexttime · 2011-07-17T19:46:59Z

Changed reviewer from Volker Braun to Volker Braun, Leif Leonhardy

jdemeyer · 2011-07-19T09:49:50Z

comment:14

New spkg, same location. This cases care of most of leif's comments. I believe that the remainder of leif's comments should not prevent a positive review for this ticket.

About committing the changes: I find it very annoying to have to commit the changes everytime I make a change. If you are happy with the spkg except for the committing of the changes, I can still commit the changes at that point.

nexttime · 2011-07-19T16:58:52Z

comment:15

Replying to @jdemeyer:

About committing the changes: I find it very annoying to have to commit the changes everytime I make a change.

You don't have to, unless you publish them. ;-)

And you can always keep an uncommitted copy. I don't think it's much work to commit the changes if you have to create (perhaps upload) and announce a new spkg anyway.

If you are happy with the spkg except for the committing of the changes, I can still commit the changes at that point.

Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg

in order to really test all changes (as files get deleted upon commit),
if I want to make some reviewer patch or a (follow-up) spkg based on yours.

The first point is of course more crucial, since not only lazy people might just install and test the spkg as is, such that potential errors will only show up much later. So not committing the changes is IMHO simply bad practice unless the spkg is really and explicitly said to be in some alpha state, most probably not ready and not intended to get merged as is (despite needing review / testing by others), which might be ok in rare cases.

There's a reason the term commit is used rather than just e.g. save changes, the former implying some confidence or conviction by the author (or committer).

jdemeyer · 2011-07-19T17:23:07Z

comment:16

Replying to @nexttime:

Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg in order to really test all changes (as files get deleted upon commit),

This is simply false. Committing does not delete the files. Committing only changes files inside .hg.

jdemeyer · 2011-07-19T17:28:21Z

comment:17

Replying to @nexttime:

Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg if I want to make some reviewer patch or a (follow-up) spkg based on yours.

When doing this, committing the changes is probably the least of your worries. Basing a new spkg on a not-yet-positively-reviewed skpg is not a very good idea anyway (see #11605 vs #11130).

nexttime · 2011-07-19T18:11:49Z

comment:18

From first glance:

(GNU) patch should be added to the dependencies section.
This is still in:

# If SAGE_DEBUG is set either unset (the default), or set to 'yes'
...
   echo >&2 "Code will be built with debugging information present. Set 'SAGE_DEBUG' to 'no' if you don't want that."
   # Actually anything othe than 'yes' will cause 
   # no debugging information to be added.

(As mentioned, I also would add -g by default.)

There's neither a patches/README.txt nor a Patches section in SPKG.txt.
success 'install' should perhaps be success 'copying header files'.

The rest looks ok.

I've btw. successfully tested the previous p7 with PARI 2.5.0.p0 and Sage 4.7.1.rc0 (Ubuntu 10.04 x86_64, GCC 4.5.1), both with our current MPIR and the newer MPIR 2.1.3 from #8664.

jdemeyer · 2011-09-22T19:57:13Z

comment:37

As said before, I don't see much reason to support older versions of PARI. The code would only get more complicated without much gain.

jdemeyer · 2011-09-26T14:05:11Z

comment:38

leif, do you still plan to make a follow-up spkg? I don't mind making a new spkg myself based on your suggestions, I just need to know how to move ahead.

nexttime · 2011-09-26T14:14:01Z

comment:39

Replying to @jdemeyer:

leif, do you still plan to make a follow-up spkg? I don't mind making a new spkg myself based on your suggestions, I just need to know how to move ahead.

I'll look at it right now, as I don't exactly recall what the remaining changes were.

I still don't think we should break Lcalc compatibility with older PARI versions for no reason; the change to make it work with both is quite trivial, and we should submit it upstream anyway.

nexttime · 2011-09-26T14:55:19Z

comment:40

Replying to @nexttime:

I'll look at it right now, as I don't exactly recall what the remaining changes were.

Oh, it seems all my changes are already committed (July 30th btw.), unfortunately all in yet another p8. I'll try to separate them into a p9, then you can make whatever changes you want (e.g. reverting the removal of overlong lines), and provide a p10.

IMHO it's very convenient to have at least an overview of the patches in SPKG.txt; IIRC, I spent quite some time recovering and documenting all the changes (mentioning just those that are still current) made over time on various tickets, of which at least some weren't referenced from SPKG.txt / the changelog, nor commit messages. (P.S.: Just looked into patches/, there's no README.patches at all.)

jdemeyer · 2011-09-26T14:58:46Z

comment:41

Replying to @nexttime:

P.S.: Just looked into patches/, there's no README.patches at all.)

My apologies, I got confused with the pari spkg, which does have a patches/README.

nexttime · 2011-09-26T15:00:15Z

comment:42

Replying to @nexttime:

[...] This does not yet fix the hardcoded g++ [...]

This seems to still apply.

I guess this was what was delaying my spkg, i.e., why I didn't upload it.

[...] eventually along with a p9 spkg with the other two superfluous upstream files deleted. [...]

Not sure about that, but I presumably already did; will check this.

nexttime · 2011-09-26T15:01:59Z

comment:43

Replying to @jdemeyer:

Replying to @nexttime:

P.S.: Just looked into patches/, there's no README.patches at all.)

I got confused with the pari spkg, which does have a patches/README.

Me too... ;-)

jdemeyer · 2011-10-06T09:36:04Z

comment:45

New spkg containing most (not all) of leif's changes:
http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-1.23.p9.spkg

Needs review.

jdemeyer · 2011-10-06T09:40:33Z

Attachment: lcalc-1.23.p8-p9.diff.gz

Attachment: lcalc-1.23.p6-p9.diff.gz

jdemeyer · 2011-10-08T09:28:38Z

Changed author from Jeroen Demeyer to Jeroen Demeyer, Leif Leonhardy

williamstein · 2011-11-03T00:29:14Z

comment:48

I read through the diff to the spkg, and everything looked reasonable to me. I applied the new pari stuff (#11330), then build this spkg, and it worked fine, and passed the full test suite (for Sage) after applying it.

I tried one simple example "by hand" that actually uses lcalc, and was not pleased by what happened:

sage: E = EllipticCurve('37a')
sage: L = E.lseries()
sage: L.zeros(10)
  ***   Warning: new stack size = 1030944 (0.983 Mbytes).
[0.000000000, 5.00317001, 6.87039122, 8.01433081, 9.93309835, 10.7751382, 11.7573247, 12.9583864, 15.6038579, 16.1920174]
sage: L.zeros(10)
  ***   Warning: new stack size = 1030944 (0.983 Mbytes).
[0.000000000, 5.00317001, 6.87039122, 8.01433081, 9.93309835, 10.7751382, 11.7573247, 12.9583864, 15.6038579, 16.1920174]

Basically, every time you use lcalc to do anything with elliptic curve L-series, you get some mysterious warning (of course, really output from PARI). However, the problem has been in released Sage for a long time, so it should be a separate ticket. It was in sage-4.7. That's now #11985.

So I say: positive review for this ticket. Great work guys for cleaning up all kinds of little issues. This is not an easy spkg, to put it mildly.

jdemeyer · 2011-11-03T09:09:31Z

Changed reviewer from Volker Braun, Leif Leonhardy to Volker Braun, Leif Leonhardy, William Stein

jdemeyer · 2011-11-03T09:09:31Z

comment:49

Thanks a lot for the review.

jdemeyer · 2011-11-03T16:14:43Z

Milestone sage-4.7.3 deleted

jdemeyer · 2011-11-07T10:11:08Z

Merged: sage-4.8.alpha1

jdemeyer added this to the sage-4.7.1 milestone May 10, 2011

jdemeyer added c: packages: standard labels May 10, 2011