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:  sage7.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 )
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/eclib20170104.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
comment:2 Changed 3 years ago by
 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 followup: ↓ 4 Changed 3 years ago by
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
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 enablempfp 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 enablempfp set and hence NTL_ALL set, you have to turn it off using disablempfp. Since Sage's spkginstall 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
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 1 1 # distutils: language = c++ 2 # distutils: extra_compile_args = DNTL_ALL3 2 # distutils: libraries = ec ntl pari gmp m 4 3 5 4 # NOTE: eclib includes have specific dependencies and must be included
comment:6 Changed 3 years ago by
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
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 followup: ↓ 9 Changed 3 years ago by
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
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
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 reused; 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
 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
Do you mean you want to keep working on eclib
itself or on the Sage interface?
comment:13 Changed 3 years ago by
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
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
 Description modified (diff)
comment:16 Changed 3 years ago by
 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
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 followup: ↓ 19 Changed 3 years ago by
 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
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
 Commit changed from b23e8cfe387df28de043ed26097449a5b1db75e9 to 57e1ae4c74e0667b0241068ff2a88b75165e0856
Branch pushed to git repo; I updated commit sha1. New commits:
57e1ae4  updated eclib to v20161223

comment:21 Changed 3 years ago by
 Description modified (diff)
comment:22 Changed 3 years ago by
 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
 Commit changed from 57e1ae4c74e0667b0241068ff2a88b75165e0856 to 13662b2a16c99a2324cba2bb9b5d4385bad7acee
Branch pushed to git repo; I updated commit sha1. New commits:
13662b2  updated eclib to v20161230

comment:24 Changed 3 years ago by
 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
 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
 Status changed from needs_review to positive_review
comment:27 Changed 3 years ago by
 Status changed from positive_review to needs_work
Very sorry, JD  see #10256
comment:28 Changed 3 years ago by
 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
 Commit changed from 13662b2a16c99a2324cba2bb9b5d4385bad7acee to 7e695e8c6ce01db84035d1206e615661d9678e5f
Branch pushed to git repo; I updated commit sha1. New commits:
7e695e8  updated eclib to v20170103

comment:30 Changed 3 years ago by
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
I fear I am losing all credibility here. Scrap v20170103 and watch this space.
comment:32 Changed 3 years ago by
 Commit changed from 7e695e8c6ce01db84035d1206e615661d9678e5f to 1c7887157a6d7ae068c28637152c56e3d8c00804
comment:33 Changed 3 years ago by
 Description modified (diff)
 Summary changed from Upgrade eclib to version 20170103 to Upgrade eclib to version 20170104
comment:34 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:35 Changed 3 years ago by
 Cc wuthrich added
comment:36 Changed 3 years ago by
Can you squash all those commits? I see little point in keeping all those incremental upgrades as separate commits.
comment:37 followup: ↓ 38 Changed 3 years ago by
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
comment:39 Changed 3 years ago by
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 nongit 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
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
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
 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
 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
Thanks. Now I'll replace the branch at #10256 with a tidied one too.
comment:45 Changed 3 years ago by
 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.
comment:46 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:47 Changed 3 years ago by
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
Good. I'll try to review this today.
comment:49 Changed 3 years ago by
 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
Thanks. The mathematical changes are effectively reviewed (already) at #10256.
comment:51 Changed 3 years ago by
 Branch changed from u/cremona/22077new to cb3fd05c95fb3afbaed9ba621038293b5412d6e0
 Resolution set to fixed
 Status changed from positive_review to closed
I will upload a branch as soon as I have tested the new build and all doctests.