Opened 3 years ago

Closed 3 years ago

#22077 closed enhancement (fixed)

Upgrade eclib to version 20170104

Reported by: cremona Owned by:
Priority: major Milestone: sage-7.5
Component: packages: standard Keywords: eclib
Cc: wuthrich Merged in:
Authors: John Cremona Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: cb3fd05 (Commits) Commit: cb3fd05c95fb3afbaed9ba621038293b5412d6e0
Dependencies: Stopgaps:

Description (last modified by cremona)

The current eclib version in Sage (as of 7.5.beta5) is v20160720.

New tarball: http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20170104.tar.bz2

Of relevance to Sage are a bugfix and some extra functionality related to modular symbols, see #10256 and #21046.

Change History (51)

comment:1 Changed 3 years ago by cremona

I will upload a branch as soon as I have tested the new build and all doctests.

comment:2 Changed 3 years ago by cremona

  • Branch set to u/cremona/22077
  • Commit set to 089877e25157a83b74de930c553bc3eea32884fa
  • Status changed from new to needs_review

OK, please review. As well as the new package code I made some small changes to the output of some modular symbol doctests. In each case I checked that the change is exactly that which one would expect from the new eclib code (a few integers fivided by 2).

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

So, what about https://github.com/JohnCremona/eclib/issues/10

Do we still need the NTL_ALL macro?

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

Replying to jdemeyer:

So, what about https://github.com/JohnCremona/eclib/issues/10

Do we still need the NTL_ALL macro?

If you look here: https://github.com/JohnCremona/eclib/commits/master, specifically at the commits made in January this year, you will see that the code now works (at least it is supposed to) with or without NTL_ALL being set. This is controlled by the --enable-mpfp flag to ./configure. That was quite a lot of work since there were quite a lot of tests whose output was trivially different with/without that option set.

Note that the default configurations is to have --enable-mpfp set and hence NTL_ALL set, you have to turn it off using --disable-mpfp. Since Sage's spkg-install does not turn it off, it is on in Sage.

So I think the only thing I did not do is update and close that Issue. Sorry!

comment:5 Changed 3 years ago by jdemeyer

If so, can you apply this additional patch

  • src/sage/libs/eclib/__init__.pxd

    diff --git a/src/sage/libs/eclib/__init__.pxd b/src/sage/libs/eclib/__init__.pxd
    index a666ec1..e42d0cc 100644
    a b  
    11# distutils: language = c++
    2 # distutils: extra_compile_args = -DNTL_ALL
    32# distutils: libraries = ec ntl pari gmp m
    43
    54# NOTE: eclib includes have specific dependencies and must be included

comment:6 Changed 3 years ago by jdemeyer

Regarding the changed doctests, I cannot really review those. But I'm happy to assume that you know what you are doing and just accept the changed output.

comment:7 Changed 3 years ago by cremona

OK, I can make that additional change. After that I'll force a rebuild of eclib in Sage and watch: it should be that -DNTL_ALL is set during compilation anyway. However I just looked at the log from my build of the new spkg yesterday and every compile line only contains "-D NTL_ALL" and no other instances of "NTL_ALL".

About the doctests: I ran them past Rob Pollack and Chris Wuthrich who are experts, and also the remarks I made above were supposed to be a justification for the correctness of the new values.

comment:8 follow-up: Changed 3 years ago by cremona

After writing that I realised that I did not know when the presence of the line you suggest deleting has any effect. It has nothing to do with building libec at build time, I realise, but rather is there to make sure that the cython compilations which read eclib's header files have the right macros set. And that must be crucial given that the compiled code must then link to the library libec! So I now think that we must continue to set NTL_ALL in this cython file, otherwise there will be errors which including eclib's header files.

I checked to see what other compiler flags are set at eclib build time. There is one other (apart from lots put there by autotools): FLINT_LEVEL=1. But that one is not an issue since it controls some implementations (in some eclib .cc files) and not headers.

Does this make sense? If so, no further patching is needed.

comment:9 in reply to: ↑ 8 Changed 3 years ago by jdemeyer

Replying to cremona:

It has nothing to do with building libec at build time, I realise, but rather is there to make sure that the cython compilations which read eclib's header files have the right macros set.

Exactly. This is about compiling Sage library files (Cython extensions) against libec.

And that must be crucial given that the compiled code must then link to the library libec! So I now think that we must continue to set NTL_ALL in this cython file, otherwise there will be errors which including eclib's header files.

That's a pity because -DNTL_ALL should not be needed. That was precisely the point that I was trying to make at https://github.com/JohnCremona/eclib/issues/10: eclib should not force other packages (like Sage) to use certain compiler flags. The NTL_ALL macro should be considered an implementation detail of eclib and it should not influence other software built against eclib.

But this is a minor point which won't influence this Sage ticket.

comment:10 Changed 3 years ago by cremona

I understand your point, but it is more than just an implementation detail, as it affects the interface. When compiled with the NTL_ALL option, functions which compute real or complex values (e.g. periods of elliptic curves) do so to arbitrary precision, returning values with type RR (NTL reals) for example, while without that option the same named function will return a C double.

I do understand that this can be viewed as bad design on my part and not a feature which a professional library should have. eclib has undergone a lot of transition since it was just "my code" (until 2007) so that by now it is almost respectable. One of the remaining issues is this one: users should not have to decide at build time whether they want to use doubles or arbitrary precision for a large collection of functions; the library should provide both and allow the uers to use whichever is most appropriate in the circumstances. In some cases, because of overloading, the same function name can be re-used; but in others different names would be necessary (e.g. for periods of elliptic curves, where the input is the same exact data whether ones wants low or high precision). It will happen one day.

I will copy some of this discussions to https://github.com/JohnCremona/eclib/issues/10

comment:11 Changed 3 years ago by cremona

  • Status changed from needs_review to needs_work

I am resetting this to "needs work" just so I can work some more on adding functions to make the interface for modular symbols better as in #10256. (There was also a test output file committed to the last eclib release by mistake). It looks as if this missed the 7.5 release anyway.

comment:12 Changed 3 years ago by jdemeyer

Do you mean you want to keep working on eclib itself or on the Sage interface?

comment:13 Changed 3 years ago by cremona

Both. There are normalization issues around modular symbols that I am sorting out on the eclib side as well as giving access to them via Sage. So there will be a new eclib release (possibly today if I can finish) and at the same time I have branch for #10256 which will depend on that (hence #10256 has this ticket as a dependency).

comment:14 Changed 3 years ago by jdemeyer

Fine for me. In case you want to change your mind, feel free to set this back to needs_review.

comment:15 Changed 3 years ago by cremona

  • Description modified (diff)

comment:16 Changed 3 years ago by git

  • Commit changed from 089877e25157a83b74de930c553bc3eea32884fa to b23e8cfe387df28de043ed26097449a5b1db75e9

Branch pushed to git repo; I updated commit sha1. New commits:

b23e8cf#22077: upgrade eclib to v20161222

comment:17 Changed 3 years ago by cremona

OK, so here is another new version. I don't expect to need to make more changes as I work on #10256. On the other hand there is perhaps little point in getting this in before that anyway: I made some more changes to modular symbol doctest output just so they pass but those tests will all be reworked in #10256 anyway. It would not hurt for someone else to check that the new release still works OK rather than having to deal with both tickets at once. I don't know. Anyway, #10256 will depend on this as it is now.

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

  • Description modified (diff)
  • Summary changed from Upgrade eclib to version 20161219 to Upgrade eclib to version 20161222

Needs review then?

comment:19 in reply to: ↑ 18 Changed 3 years ago by cremona

Replying to jdemeyer:

Needs review then?

Not yet. I uploaded that new version last night but then realised that I have yet to ensure that the modular symbols are normalised as to sign (I dealt with the factor of 2 ok, touch wood). This probably means a delay of several days now...

comment:20 Changed 3 years ago by git

  • Commit changed from b23e8cfe387df28de043ed26097449a5b1db75e9 to 57e1ae4c74e0667b0241068ff2a88b75165e0856

Branch pushed to git repo; I updated commit sha1. New commits:

57e1ae4updated eclib to v20161223

comment:21 Changed 3 years ago by cremona

  • Description modified (diff)

comment:22 Changed 3 years ago by cremona

  • Description modified (diff)

With luck this version (v20161230) will be the last bugfix version with the new modular symbols functionality. Right now this all works fine with the branch soon to be uploaded to #10256, but I should make a version here which passes all tests even if the relevant tests will all be superseded there.

comment:23 Changed 3 years ago by git

  • Commit changed from 57e1ae4c74e0667b0241068ff2a88b75165e0856 to 13662b2a16c99a2324cba2bb9b5d4385bad7acee

Branch pushed to git repo; I updated commit sha1. New commits:

13662b2updated eclib to v20161230

comment:24 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

Please review, using v20161230 of eclib (as in the link). The only doctest changes are to do with scaling of modular symbols, which will be dealt with properly in #10256, so the only thing to do is check that the new source builds OK and all test pass.

comment:25 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Summary changed from Upgrade eclib to version 20161222 to Upgrade eclib to version 20161230

comment:26 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:27 Changed 3 years ago by cremona

  • Status changed from positive_review to needs_work

Very sorry, JD -- see #10256

comment:28 Changed 3 years ago by cremona

  • Description modified (diff)
  • Summary changed from Upgrade eclib to version 20161230 to Upgrade eclib to version 20170103

Another version will appear shortly. For the record: the sign of plus modular symbols was being set correctly when L(E,1) is nonzero but not otherwise (random) and the sign of the minus symbol was not being set at all. Now both are dealt with.

The new tarball is in place, and I will update the branch soon.

I suggest that when this has yet again had a review by Jeroen, we do not set it to positive review until Chris is happy over at #10256.

comment:29 Changed 3 years ago by git

  • Commit changed from 13662b2a16c99a2324cba2bb9b5d4385bad7acee to 7e695e8c6ce01db84035d1206e615661d9678e5f

Branch pushed to git repo; I updated commit sha1. New commits:

7e695e8updated eclib to v20170103

comment:30 Changed 3 years ago by cremona

OK, ready for review again. Apart from a general check that nothing is broken this should wait until #10256 is also good.

comment:31 Changed 3 years ago by cremona

I fear I am losing all credibility here. Scrap v20170103 and watch this space.

comment:32 Changed 3 years ago by git

  • Commit changed from 7e695e8c6ce01db84035d1206e615661d9678e5f to 1c7887157a6d7ae068c28637152c56e3d8c00804

Branch pushed to git repo; I updated commit sha1. New commits:

8fabe91updated eclib to v20170104
1c78871updated eclib to v20170104 again

comment:33 Changed 3 years ago by cremona

  • Description modified (diff)
  • Summary changed from Upgrade eclib to version 20170103 to Upgrade eclib to version 20170104

Another attempt, with more testing this time.


New commits:

8fabe91updated eclib to v20170104
1c78871updated eclib to v20170104 again

comment:34 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

comment:35 Changed 3 years ago by wuthrich

  • Cc wuthrich added

comment:36 Changed 3 years ago by jdemeyer

Can you squash all those commits? I see little point in keeping all those incremental upgrades as separate commits.

comment:37 follow-up: Changed 3 years ago by wuthrich

Probably, I would not be happy at #10256 if you squash commits here. Does it really harm having them?

comment:38 in reply to: ↑ 37 Changed 3 years ago by jdemeyer

Replying to wuthrich:

Probably, I would not be happy at #10256 if you squash commits here.

Never mind then.

comment:39 Changed 3 years ago by cremona

Yes, sorry about the tangle. We certainly could disentangle: on this ticket that's what you were suggesting anyway, and on the other one I think there are <6 files affected and one could always use the non-git route of copying those 6 files elsewhere first and starting a new branch... I can do that if necessary.

comment:40 Changed 3 years ago by cremona

I made a clean single commit on top of 7.5.rc1 which is at u/cremona/22077new. It could be used instead of the original u/cremona/22077, but only if #10256 is also cleaned up. Next...

comment:41 Changed 3 years ago by cremona

Jeroen, how about using my clean branch here, and at #10256 using one I just made which merges this with the current branch there (u/wuthrich/ticket/10256)? I resolved the merge conflicts in 3 files, so now have a version of that branch which is no worse than before, while this one is simpler.

comment:42 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

That's all fine for me. Just change branches and set this one back to needs_review.

comment:43 Changed 3 years ago by wuthrich

  • Branch changed from u/cremona/22077 to u/cremona/22077new
  • Commit changed from 1c7887157a6d7ae068c28637152c56e3d8c00804 to cb3fd05c95fb3afbaed9ba621038293b5412d6e0
  • Status changed from needs_work to needs_review

New commits:

cb3fd05#22077 update eclib to v20170104

comment:44 Changed 3 years ago by cremona

Thanks. Now I'll replace the branch at #10256 with a tidied one too.

comment:45 Changed 3 years ago by cremona

  • Status changed from needs_review to needs_work

Apologies, I messed up -- the new branch is certainly a cleaned up version of its predecessor but that one was wrong. I will fix this in the morning.

Ignore the above comment, this (=branch u/cremona/22077new = commit cb3fd05) is fine. Just note that the additional work on #10256 will involve changes to sage/libs/eclib as well as sage/schemes/elliptic_curves, in order to expose some new functionality in eclib. This one *only* does the minimum to make eclib c20170104 work OK and pass tests.

Last edited 3 years ago by cremona (previous) (diff)

comment:46 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

comment:47 Changed 3 years ago by cremona

Note that #10256 which depends on this has a postive review. I don't know if this proprty is inherited!

comment:48 Changed 3 years ago by jdemeyer

Good. I'll try to review this today.

comment:49 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

I didn't check the mathematics. But the package builds fine.

comment:50 Changed 3 years ago by cremona

Thanks. The mathematical changes are effectively reviewed (already) at #10256.

comment:51 Changed 3 years ago by vbraun

  • Branch changed from u/cremona/22077new to cb3fd05c95fb3afbaed9ba621038293b5412d6e0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.