Opened 4 years ago

Closed 3 years ago

#11321 closed defect (fixed)

Make lcalc compatible with the new PARI

Reported by: jdemeyer Owned by: jdemeyer
Priority: major Milestone: sage-4.8
Component: packages: standard Keywords: lcalc spkg
Cc: fbissey, rishi Merged in: sage-4.8.alpha1
Authors: Jeroen Demeyer, Leif Leonhardy Reviewers: Volker Braun, Leif Leonhardy, William Stein
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: Commit:
Dependencies: #11130 Stopgaps:

Description (last modified by jdemeyer)

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 lcalc-1.23.p6-p9.diff to see the difference with the lcalc spkg currently in Sage.

Attachments (4)

trac_11321-reviewer_part1-document_patches_and_further_clean-up.spkg.patch (15.2 KB) - added by leif 3 years ago.
SPKG patch. The only change with slight functional effect is the rearrangement of flags (CFLAGS et al.). Apply to Jeroen's p8 of July 26th.
trac_11321-reviewer_part2-support_older_PARI-improve_doc.spkg.patch (5.2 KB) - added by leif 3 years ago.
SPKG patch. Makes this spkg also work with older PARI versions (prior to 2.4.4), adds some notes to SPKG.txt. Apply on top of "part_1".
lcalc-1.23.p8-p9.diff (27.6 KB) - added by jdemeyer 3 years ago.
lcalc-1.23.p6-p9.diff (54.6 KB) - added by jdemeyer 3 years ago.

Download all attachments as: .zip

Change History (55)

comment:1 follow-up: Changed 4 years ago by vbraun

  • Reviewers set to Volker Braun

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

comment:2 Changed 4 years ago by fbissey

  • Cc fbissey added

comment:3 in reply to: ↑ 1 Changed 4 years ago by jdemeyer

Replying to vbraun:

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

Yes, when I have time.

comment:4 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 4 years ago by jdemeyer

  • Dependencies set to #11130

comment:6 Changed 4 years ago by jdemeyer

  • Report Upstream changed from N/A to Reported upstream. Little or no feedback.

comment:7 Changed 4 years ago by jdemeyer

  • Status changed from new to needs_review

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

comment:8 Changed 4 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.

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

comment:9 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:10 Changed 4 years ago by jdemeyer

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

comment:11 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 3 years ago by leif

  • Description modified (diff)

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.

comment:13 Changed 3 years ago by leif

  • Reviewers changed from Volker Braun to Volker Braun, Leif Leonhardy

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. #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.

comment:14 follow-up: Changed 3 years ago by jdemeyer

  • Description modified (diff)

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.

comment:15 in reply to: ↑ 14 ; follow-ups: Changed 3 years ago by leif

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).

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by jdemeyer

Replying to leif:

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.

comment:17 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by jdemeyer

Replying to leif:

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).

comment:18 follow-ups: Changed 3 years ago by leif

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.

comment:19 in reply to: ↑ 16 ; follow-up: Changed 3 years ago by leif

Replying to jdemeyer:

Replying to leif:

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.

hg remove foo && hg commit removes foo also from the working directory (unless you've already deleted it).

comment:20 in reply to: ↑ 17 Changed 3 years ago by leif

Replying to jdemeyer:

Replying to leif:

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.

For a reviewer patch it is.

Basing a new spkg on a not-yet-positively-reviewed skpg is not a very good idea anyway (see #11605 vs #11130).

#11604 was based on #11605 in the hope of (or giving it a chance) getting it into Sage 4.7.1, such that this bug gets fixed in reasonable time (in an official release). #11605 was likely to get merged into 4.7.1 because it is a critical bug, so not basing #11604 on the former would have simply been stupid.

#11130 has the milestone set to Sage 4.7.2 and currently (officially) still needs work. (Basing a new spkg on that of #11130 would not have been necessary since the new PARI version fixes the bug anyway. And even if it didn't, you claim above it would have been a bad idea to base a new one on a not yet positively reviewed, as is the case with PARI 2.5.0.p0, more precisely, it even "needs work" as I mentioned above.)

comment:21 in reply to: ↑ 19 Changed 3 years ago by leif

Replying to leif:

Replying to jdemeyer:

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

hg remove foo && hg commit removes foo also from the working directory (unless you've already deleted it).

P.S.: If you don't want the file in the working directory to get deleted, use hg forget ..., which just removes it from the (current version in the) repository.

comment:22 Changed 3 years ago by leif

Ok, you're right. hg remove foo removes foo from the working directory immediately. (And if you recreate it and commit, that won't delete the new one either, which might be a different pitfall, though Mercurial will report it as an unknown file afterwards.)

Anyway, I believe hg status should simply report nothing, as one might miss the ? among lots of other files marked M, A or R.

comment:23 Changed 3 years ago by leif

P.P.S.: src/include/Lvalue.h.bak is still present in the p8.

comment:24 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to leif:

  • 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.)

I do not plan to change this here. This is a functional change which is unrelated to the topic of this ticket so such a change does not belong here.

comment:25 in reply to: ↑ 24 Changed 3 years ago by leif

Replying to jdemeyer:

Replying to leif:

  • 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.)

I do not plan to change this here. This is a functional change which is unrelated to the topic of this ticket so such a change does not belong here.

Just remove the "is set either unset (the default), or", add "or unset it" to "Set 'SAGE_DEBUG' to 'no' ...", and s/othe/other/.

I don't mind not adding -g by default here (though it really makes sense, and other packages do this). Of course one can always put -g into the "global" CFLAGS, but you usually need debugging information when it's "too late". Getting better tracebacks from less experienced users is the main reason. One can always strip the symbols afterwards...

Also, other packages that disable optimization completely (-O0) if SAGE_DEBUG=yes should IMHO issue a warning when doing so.

comment:26 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2
  • Status changed from needs_work to needs_review

New spkg (even with changes committed), same location: http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-1.23.p8.spkg

comment:27 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by leif

This still applies:

Replying to leif:

  • (GNU) patch should be added to the dependencies section.
  • There's neither a patches/README.txt nor a Patches section in SPKG.txt.
  • success 'install' should perhaps be success 'copying header files'.

I'll perhaps add a reviewer patch for this, which of course doesn't affect that:

P.P.S.: src/include/Lvalue.h.bak is still present in the p8.

But it's mentioned in the Special Update/Build? Instructions, and only about 25 KB, so a minor issue.

comment:28 Changed 3 years ago by leif

Just found some more things:

  • src/src/Makefile.old can also be deleted.
  • The Makefile not only defines CC to g++, but also hardcodes g++ when linking the shared library.

comment:29 Changed 3 years ago by leif

Any reason to not build (and run) the test suite (from an spkg-check, to be added)?

(Haven't tried, so it may be in an unusable state.)

Changed 3 years ago by leif

SPKG patch. The only change with slight functional effect is the rearrangement of flags (CFLAGS et al.). Apply to Jeroen's p8 of July 26th.

comment:30 in reply to: ↑ 27 Changed 3 years ago by leif

Replying to leif:

This still applies:

Replying to leif:

  • (GNU) patch should be added to the dependencies section.
  • There's neither a patches/README.txt nor a Patches section in SPKG.txt.
  • success 'install' should perhaps be success 'copying header files'.

I'll perhaps add a reviewer patch for this.

Did so. I've also made further cosmetic changes to spkg-install, and also changed the order of flags in CFLAGS etc. such that user settings only get overridden when necessary.

I'll perhaps add a second reviewer patch fixing the hardcoded g++ in Lcalc's Makefile, eventually along with a p9 spkg with the other two superfluous upstream files deleted.

Changed 3 years ago by leif

SPKG patch. Makes this spkg also work with older PARI versions (prior to 2.4.4), adds some notes to SPKG.txt. Apply on top of "part_1".

comment:31 follow-up: Changed 3 years ago by leif

  • Description modified (diff)

A second reviewer patch is up.

This does not yet fix the hardcoded g++, but changes the pari-2.4.4.patch such that it will work with older PARI versions, e.g. our current 2.4.3.alpha.* spkgs, as well, so we can in principle remove the dependency on #11130 (and merge it earlier).

I've also detailed the description of the patches in SPKG.txt and added a few more comments there.

comment:32 Changed 3 years ago by jdemeyer

Leif, will you post a new spkg with your changes added?

comment:33 in reply to: ↑ 31 ; follow-up: Changed 3 years ago by jdemeyer

Replying to leif:

changes the pari-2.4.4.patch such that it will work with older PARI versions, e.g. our current 2.4.3.alpha.* spkgs, as well, so we can in principle remove the dependency on #11130 (and merge it earlier).

I'm not sure why we should do this. There is no reason to merge this before #11130, so making it compatible with earlier versions just looks like more complication to me.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 3 years ago by leif

  • Cc rishi added

Replying to jdemeyer:

Replying to leif:
Leif, will you post a new spkg with your changes added?

Yes, later today.

changes the pari-2.4.4.patch such that it will work with older PARI versions, e.g. our current 2.4.3.alpha.* spkgs, as well, so we can in principle remove the dependency on #11130 (and merge it earlier).

I'm not sure why we should do this. There is no reason to merge this before #11130, so making it compatible with earlier versions just looks like more complication to me.

Well, it's not too complicated it wasn't worth to remove the dependency, be it for testing / reviewing, or further work on the package. (The new spkg will have other advantages btw.)

But that's IMHO something we should report upstream, as it won't be the last time Lcalc will have some PARI version-dependent parts... ;-)

comment:35 in reply to: ↑ 34 Changed 3 years ago by leif

Replying to leif:

Replying to jdemeyer:

Replying to leif:
Leif, will you post a new spkg with your changes added?

Yes, later today.

"Today" still applies, only it's Wednesday now... ;-)

comment:36 Changed 3 years ago by jdemeyer

Regardless of what leif said, I think my spkg still stands to be reviewed as-is.

leif, please do not clutter the SPKG.txt with a patches section since we have a patches/README.txt. No reason to state the same information in more than one place.

Also, I dislike

echo >&2 "fooooooooooooooo" \
         "barrrrrrrrrrrrrr"

This is much harder to read than

echo >&2 "fooooooooooooooo" "barrrrrrrrrrrrrr"

comment:37 Changed 3 years ago by jdemeyer

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.

comment:38 follow-up: Changed 3 years ago by 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.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 3 years ago by leif

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.

comment:40 in reply to: ↑ 39 ; follow-up: Changed 3 years ago by leif

Replying to leif:

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.)

comment:41 in reply to: ↑ 40 ; follow-up: Changed 3 years ago by jdemeyer

Replying to leif:

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.

comment:42 Changed 3 years ago by leif

Replying to leif:

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

comment:43 in reply to: ↑ 41 Changed 3 years ago by leif

Replying to jdemeyer:

Replying to leif:

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... ;-)

comment:44 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-4.7.2 to sage-4.7.3

comment:45 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Owner changed from tbd to jdemeyer

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.

Changed 3 years ago by jdemeyer

Changed 3 years ago by jdemeyer

comment:46 Changed 3 years ago by jdemeyer

  • Authors changed from Jeroen Demeyer to Jeroen Demeyer, Leif Leonhardy

comment:47 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:48 Changed 3 years ago by was

  • Status changed from needs_review to positive_review

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.

comment:49 Changed 3 years ago by jdemeyer

  • Reviewers changed from Volker Braun, Leif Leonhardy to Volker Braun, Leif Leonhardy, William Stein

Thanks a lot for the review.

comment:50 Changed 3 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:51 Changed 3 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha1
  • Milestone set to sage-4.8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.