Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#18845 closed enhancement (fixed)

Update NTL to 9.2.0

Reported by: jpflori Owned by:
Priority: major Milestone: sage-6.8
Component: packages: standard Keywords:
Cc: fbissey, jdemeyer Merged in:
Authors: Jean-Pierre Flori Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 4fd99b3 (Commits, GitHub, GitLab) Commit:
Dependencies: #18866, #18867 Stopgaps:

Status badges

Change History (56)

comment:1 Changed 6 years ago by jpflori

  • Description modified (diff)

comment:2 Changed 6 years ago by jpflori

  • Cc fbissey added

CCing s-o-g folk as they already worked hard to include an uptodate NTL there.

comment:3 Changed 6 years ago by jpflori

  • Branch set to u/jpflori/ticket/18845
  • Commit set to dd2285505edf4bf7b96283d7954de05333e232e6

I've updated three fuzzy/failing patching.

I've not touched the error callback patch which does not apply ass we have two solutions:

  • update our patch,
  • use the native NTL error callback mechanism, this can arguably be done elsewhere if the former solution is trivial.

There should still be trouble with flint, singular and friends. From what I read flint is fixed in git master. No clear idea about Singular. From what I read there is a fix upstream, but is it available for 3.x as we are stuck with that at the moment, or only for 4.x?


New commits:

dd22855Update some NTL patches. WIP.

comment:4 Changed 6 years ago by fbissey

flint will build against it but not singular, I am not even sure singular-4 will. I would need to check my inbox from when we looked at it in Gentoo with the guy that bumped ntl to 9.x.

comment:5 follow-up: Changed 6 years ago by fbissey

singular-4 has a fix in master: https://github.com/Singular/Sources/commit/de688442dfe449992dc14a000bca0691ecc7e106 that may be back portable.

comment:6 in reply to: ↑ 5 Changed 6 years ago by jpflori

Replying to fbissey:

singular-4 has a fix in master: https://github.com/Singular/Sources/commit/de688442dfe449992dc14a000bca0691ecc7e106 that may be back portable.

Yes, it seems it just applies as is (with offset).

comment:7 Changed 6 years ago by git

  • Commit changed from dd2285505edf4bf7b96283d7954de05333e232e6 to cedaaa41aa8b5bd123d8f399f60b553f1b7e440f

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

cedaaa4Fix NTL patches.

comment:8 Changed 6 years ago by git

  • Commit changed from cedaaa41aa8b5bd123d8f399f60b553f1b7e440f to 6611ff64f0490fde3d8e802c6cce92140e8476a3

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

6611ff6Use new style description for NTL patches.

comment:9 Changed 6 years ago by git

  • Commit changed from 6611ff64f0490fde3d8e802c6cce92140e8476a3 to ca9d43e58c82dcd6ecff92a68770e4e3c7e57b18

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

365bc00Update David Harvey's bernmm code for new NTL versions.
ca9d43eFix C++ namespaces ambiguity.

comment:10 Changed 6 years ago by jpflori

There is something wrong with my modifs to David's code. If someone could have a look, nothing jumps to me now...

comment:11 Changed 6 years ago by jpflori

My changes look pretty similar to the ones here:

except for using NTL::mulmod_t instead of NTL::wide_double.

comment:12 follow-up: Changed 6 years ago by fbissey

No idea what the difference is I am afraid.

comment:13 Changed 6 years ago by git

  • Commit changed from ca9d43e58c82dcd6ecff92a68770e4e3c7e57b18 to 79246f0d651207b38ec0333d2a499dc54cf8a5de

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

79246f0Further fixes for NTL update.

comment:14 in reply to: ↑ 12 Changed 6 years ago by jpflori

Replying to fbissey:

No idea what the difference is I am afraid.

Could you test this on a usual arch? Currently, I only have Sage installed on a POWER7...

comment:15 Changed 6 years ago by fbissey

I'll have a look on my mac.

comment:16 Changed 6 years ago by fbissey

Taking longer than expected I wasn't completely up to date to beta7 (was on beta5). Will let you know when it is all done.

comment:17 Changed 6 years ago by fbissey

OK, I almost fell like I should have done a new build from scratch.

Mirage:sage-6.8.beta5 fbissey$ ./sage -t --long --warn-long 34.4 src/sage/rings/arith.py
Running doctests with ID 2015-07-07-17-26-18-c6eb7fbc.
Git branch: ntl92
Using --optional=gcc,mpir,python2,sage,scons
Doctesting 1 file.
sage -t --long --warn-long 34.4 src/sage/rings/arith.py
**********************************************************************
File "src/sage/rings/arith.py", line 294, in sage.rings.arith.bernoulli
Failed example:
    union([len(union(x))==1 for x in vals])  # long time (depends on previous line)
Expected:
    [True]
Got:
    [False, True]
**********************************************************************
File "src/sage/rings/arith.py", line 299, in sage.rings.arith.bernoulli
Failed example:
    union([len(union(x))==1 for x in vals])  # long time (depends on previous line)
Expected:
    [True]
Got:
    [False, True]
**********************************************************************
1 item had failures:
   2 of  16 in sage.rings.arith.bernoulli
    [785 tests, 2 failures, 51.51 s]
----------------------------------------------------------------------
sage -t --long --warn-long 34.4 src/sage/rings/arith.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 51.7 seconds
    cpu time: 38.6 seconds
    cumulative wall time: 51.5 seconds
Mirage:sage-6.8.beta5 fbissey$ ./sage -t --long --warn-long 34.4 src/sage/rings/bernmm.pyx
Running doctests with ID 2015-07-07-19-02-38-d29775f0.
Git branch: ntl92
Using --optional=gcc,mpir,python2,sage,scons
Doctesting 1 file.
sage -t --long --warn-long 34.4 src/sage/rings/bernmm.pyx
**********************************************************************
File "src/sage/rings/bernmm.pyx", line 68, in sage.rings.bernmm.bernmm_bern_rat
Failed example:
    lst1 == lst2
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/rings/bernmm.pyx", line 70, in sage.rings.bernmm.bernmm_bern_rat
Failed example:
    [ Zmod(101)(t) for t in lst1 ]
Expected:
    [77, 72, 89, 98, 86]
Got:
    [99, 0, 44, 42, 57]
**********************************************************************
1 item had failures:
   2 of  13 in sage.rings.bernmm.bernmm_bern_rat
    [23 tests, 2 failures, 0.79 s]
----------------------------------------------------------------------
sage -t --long --warn-long 34.4 src/sage/rings/bernmm.pyx  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 0.8 seconds
    cpu time: 1.2 seconds
    cumulative wall time: 0.8 seconds
Mirage:sage-6.8.beta5 fbissey$ ./sage -t --long --warn-long 34.4 src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/birds_other.rst
Running doctests with ID 2015-07-07-19-02-58-71d664dd.
Git branch: ntl92
Using --optional=gcc,mpir,python2,sage,scons
Doctesting 1 file.
sage -t --long --warn-long 34.4 src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/birds_other.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/birds_other.rst", line 212, in doc.en.thematic_tutorials.explicit_methods_in_number_theory.birds_other
Warning, slow doctest:
    w2 = bernoulli(100000, algorithm='pari')   # long time (6s on sage.math, 2011)
Test ran for 55.38 s
**********************************************************************
File "src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/birds_other.rst", line 213, in doc.en.thematic_tutorials.explicit_methods_in_number_theory.birds_other
Failed example:
    w1 == w2  # long time
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of  42 in doc.en.thematic_tutorials.explicit_methods_in_number_theory.birds_other
    [34 tests, 1 failure, 62.40 s]
----------------------------------------------------------------------
sage -t --long --warn-long 34.4 src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/birds_other.rst  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 62.4 seconds
    cpu time: 67.9 seconds
    cumulative wall time: 62.4 seconds

comment:18 Changed 6 years ago by jpflori

  • Cc jdemeyer added

You get the same errors as I do.

And I cannot get NTL to build with NTL_LEGACY_SP_MULMOD=on to check that the old double implem is still fine. Build fails with #if with no expression just as in #3486 or https://groups.google.com/forum/#!topic/sage-devel/4r8HrQVOSe4

comment:19 Changed 6 years ago by jpflori

For info, the corresponding part of NTL doc is at:

comment:20 Changed 6 years ago by jpflori

My inability to try NTL_LEGACY_SP_MULMOD is caused by the fact that InitSettings does not build when it is set because of the typo:

 static inline long MulMod2(long a, long b, long n, wide_double bninv)
{
   return MulMod2_legacy(a, b, n, ninv);
}

comment:21 follow-up: Changed 6 years ago by fbissey

Upstream bug then.

comment:22 Changed 6 years ago by git

  • Commit changed from 79246f0d651207b38ec0333d2a499dc54cf8a5de to 3b1f14a3976e375ee23271a780b4e54e057549ec

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

3b1f14aFix typo in sp_arith.h.

comment:23 in reply to: ↑ 21 ; follow-up: Changed 6 years ago by jpflori

Replying to fbissey:

Upstream bug then.

Sure, I just sent a report to Victor Shoup.

Note that it does not explain the failure in bernmm code with the new sp implem in (new) NTL. It will just allow us to check that bernmm works as expected with the old sp implem in (new) NTL.

comment:24 Changed 6 years ago by jpflori

And it works just fine.

You can try NTL built with the legacy sp implem at u/jpflori/ticket/18845legacy.

comment:25 in reply to: ↑ 23 Changed 6 years ago by jpflori

Replying to jpflori:

Replying to fbissey:

Upstream bug then.

Sure, I just sent a report to Victor Shoup.

And he will fix it asap.

comment:26 follow-up: Changed 6 years ago by jdemeyer

Do you think you can fix the bernmm problem? If not, using the old the "legacy" implementation is better than not merging this ticket.

comment:27 Changed 6 years ago by jdemeyer

If this is true:

NTL provides its own error callback framework

we should use it.

comment:28 follow-up: Changed 6 years ago by jdemeyer

OK, we still need the patch to the callback, but not for the reason stated. So, please replace

NTL provides its own error callback framework, but it is simpler
than the one provided by this patch: there is no context object.

by

NTL provides its own error callback framework, but it's not suitable
for Sage since we don't want to see the error message printed, we want
the error message to be part of the callback function.

comment:29 follow-up: Changed 6 years ago by jdemeyer

I just sent an e-mail to Shoup about the callback function.

comment:30 in reply to: ↑ 26 Changed 6 years ago by jpflori

Replying to jdemeyer:

Do you think you can fix the bernmm problem? If not, using the old the "legacy" implementation is better than not merging this ticket.

I'd say not quickly and I'm away for one week starting tonight.

I really have no idea why it does not work with the current patch. The only way I see to understand this is to print everything bern_modp does to find the problem. A small couple of problematic values is k = 128 and p = 7 (or another small prime).

So unless you see some obvious mistake in what I did, we can postpone the use of the new (and recommended upstream) sp implem for later. We'll just need to remove one line in spkg-install.

comment:31 in reply to: ↑ 29 Changed 6 years ago by jpflori

Replying to jdemeyer:

I just sent an e-mail to Shoup about the callback function.

Thanks, I already mentioned that twice during our exchanges with Shoup but never really explained to him the difference between our and his error callback.

If by any chance you know why the C++ new patch for Singular is needed, I'd be happy you tell Shoup and I.

comment:32 Changed 6 years ago by jpflori

Also note that NTL now provides exception handling though I've not activated it here.

comment:33 Changed 6 years ago by git

  • Commit changed from 3b1f14a3976e375ee23271a780b4e54e057549ec to a1b327a4b22269f79b237a1a936a5abde1431712

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

9fde69eBetter description for errorcallback.patch.
a1b327aBranch to test NTL with legacy sp implem.

comment:34 in reply to: ↑ 28 Changed 6 years ago by jpflori

Replying to jdemeyer:

OK, we still need the patch to the callback, but not for the reason stated. So, please replace

NTL provides its own error callback framework, but it is simpler
than the one provided by this patch: there is no context object.

by

NTL provides its own error callback framework, but it's not suitable
for Sage since we don't want to see the error message printed, we want
the error message to be part of the callback function.

Done.

I've also merged the changes from the legacy branch.

comment:35 Changed 6 years ago by git

  • Commit changed from a1b327a4b22269f79b237a1a936a5abde1431712 to db8cf9977d8f5827371b128249f22348839872a4

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

db8cf99No c++11 for NTL without threads and exceptions.

comment:36 Changed 6 years ago by jdemeyer

Shoup answered that he will update the callback function such that it will be good enough for Sage.

comment:37 Changed 6 years ago by jpflori

  • Authors set to Jean-Pierre Flori
  • Status changed from new to needs_review

comment:38 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work
Traceback (most recent call last):
  File "/usr/local/src/sage-config/src/doc/common/builder.py", line 1619, in <module>
    import sage.all
  File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/all.py", line 103, in <module>
    import sage.symbolic.pynac
ImportError: /usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/symbolic/pynac.so: undefined symbol: _ZN3NTL5ErrorEPKc

comment:39 follow-up: Changed 6 years ago by jdemeyer

It's a dependency problem: that module should depend on NTL, but it doesn't.

comment:40 Changed 6 years ago by jpflori

Strange:

[sage.git]$ ./sage 
┌────────────────────────────────────────────────────────────────────┐
│ SageMath Version 6.8.beta7, Release Date: 2015-07-02               │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: import sage.symbolic.pynac
sage:

and

[sage.git]$ nm local/lib/python/site-packages/sage/symbolic/pynac.so |grep NTL
000000000000c610 t 00000058.plt_call._ZN3NTL13TerminalErrorEPKc+0
000000000000d088 t 00000058.plt_call._ZN3NTL14BlockConstructEPNS_4ZZ_pEl+0
000000000000ca20 t 00000058.plt_call._ZN3NTL3VecINS_4ZZ_pEE10AllocateToEl+0
000000000000c7a0 t 00000058.plt_call._ZN3NTL3VecINS_5ZZ_pEEE10AllocateToEl+0
000000000000d074 t 00000058.plt_call._ZN3NTL5ZZ_pX9normalizeEv+0
000000000000c55c t 00000058.plt_call._ZN3NTL6ZZ_pEX9normalizeEv+0
000000000000c764 t 00000058.plt_call._ZNK3NTL11ZZ_pContext7restoreEv+0
0000000000078cf0 D _Z19ZZ_pEX_conv_modulusRN3NTL6ZZ_pEXERKS0_RKNS_11ZZ_pContextE
                 U _ZN3NTL13TerminalErrorEPKc
                 U _ZN3NTL14BlockConstructEPNS_4ZZ_pEl
0000000000078cd0 W _ZN3NTL3VecINS_4ZZ_pEE10AllocateToEl
0000000000078ce0 W _ZN3NTL3VecINS_5ZZ_pEEE10AllocateToEl
                 U _ZN3NTL5ZZ_pX9normalizeEv
                 U _ZN3NTL6ZZ_pEX9normalizeEv
                 U _ZN3NTL8ZZ_pInfoE
                 U _ZNK3NTL11ZZ_pContext7restoreEv

Plain Error does not exist anymore in NTL 9.x.

comment:41 in reply to: ↑ 39 Changed 6 years ago by jpflori

Replying to jdemeyer:

It's a dependency problem: that module should depend on NTL, but it doesn't.

I see. Can you fix this or should I do it?

comment:42 Changed 6 years ago by jdemeyer

Hmm, it's strange that this module isn't actually linked against ntl...

Version 0, edited 6 years ago by jdemeyer (next)

comment:43 Changed 6 years ago by jdemeyer

I'm lost here... regardless of this ticket, the module sage.symbolic.pynac is not linked against NTL, but it does contain those NTL symbols.

comment:44 Changed 6 years ago by jpflori

C++ templates black magic ? :)

Actually I had a quick look at the pyx file and found nothing about NTL...

comment:45 Changed 6 years ago by fbissey

Had a quick look earlier this morning, the only way ntl can get pulled is by a chain of include files that are not obvious to follow. sage.symbolic.pynac import some stuff from sage.rings and I think something in rings.pxd import some ntl stuff eventually. I will look for where precise NTL symbols are used next.

comment:46 Changed 6 years ago by jpflori

I'll be in a hidden valley for one week and though I might reply here, I won't have any ssh connection and any chance to fix this, so feel free to close the ticket without my help!

comment:47 Changed 6 years ago by jdemeyer

It seems that the module is really underlinked:

$ ./sage --python
Python 2.7.9 (default, Jul  5 2015, 21:49:10) 
[GCC 4.8.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sage.symbolic.pynac
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: /usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/symbolic/pynac.so: undefined symbol: _ZN3NTL8ZZ_pInfoE

The only reason there is no error in Sage is that NTL was imported before (by some other module imported earlier).

comment:48 Changed 6 years ago by jdemeyer

  • Dependencies set to #18866

There are many modules with the same linking problem. I think it's pointless to fix that problem without cleaning up the whole NTL interface (which is a big mess).

That leaves the problem of the dependency checking: I opened #18866 for that.

comment:49 Changed 6 years ago by jdemeyer

  • Branch changed from u/jpflori/ticket/18845 to u/jdemeyer/ticket/18845

comment:50 Changed 6 years ago by jdemeyer

  • Commit changed from db8cf9977d8f5827371b128249f22348839872a4 to 50a36992701a707aaa319bcf447a833b4ea600df

New commits:

3892efcModules using NTL should depend on NTL
8f3d9a5Merge remote-tracking branch 'trac/u/jpflori/ticket/18845' into ticket/18845
50a3699Change version_stamp to support downgrading NTL

comment:51 Changed 6 years ago by jdemeyer

  • Dependencies changed from #18866 to #18866, #18867

comment:52 Changed 6 years ago by git

  • Commit changed from 50a36992701a707aaa319bcf447a833b4ea600df to 4fd99b370fca63dd04213519f614df7721314fba

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

71ffefcRevert all changes to bernmm
4e39fd6Fix C++ namespaces ambiguity.
4fd99b3Merge remote-tracking branch 'trac/u/jdemeyer/ticket/18867' into ticket/18845

comment:53 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

All doctests pass (but I haven't actually reviewed the changes)

comment:54 Changed 6 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:55 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/18845 to 4fd99b370fca63dd04213519f614df7721314fba
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:56 Changed 6 years ago by jdemeyer

  • Commit 4fd99b370fca63dd04213519f614df7721314fba deleted

Follow-up: #18875

Note: See TracTickets for help on using tickets.