Opened 3 years ago

Closed 2 years ago

#22984 closed enhancement (fixed)

Upgrade normaliz to 3.5.3 and pynormaliz to 1.12

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-8.3
Component: packages: optional Keywords: IMA-PolyGeom
Cc: Winfried, tmonteil, vdelecroix, tscrim, jipilab, gh-sebasguts Merged in:
Authors: Matthias Koeppe Reviewers: Travis Scrimshaw, Thierry Monteil, Jean-Philippe Labbé
Report Upstream: Completely fixed; Fix reported upstream Work issues:
Branch: 7057ef4 (Commits) Commit: 7057ef457e5e6f78197100f938b8dae8d8b0c5a4
Dependencies: Stopgaps:

Change History (66)

comment:1 Changed 3 years ago by mkoeppe

  • Description modified (diff)
  • Milestone changed from sage-8.0 to sage-8.1
  • Summary changed from Upgrade normaliz to 3.3.0 and pynormaliz to 1.7 to Upgrade normaliz to 3.4.0 and pynormaliz to 1.7

comment:2 Changed 3 years ago by mkoeppe

  • Branch set to u/mkoeppe/upgrade_normaliz_to_3_4_0_and_pynormaliz_to_1_7

comment:3 Changed 3 years ago by mkoeppe

  • Cc tmonteil added
  • Commit set to a6ccc4235fa65a26fd772ad963df01b9ece5633f
  • Description modified (diff)
  • Summary changed from Upgrade normaliz to 3.4.0 and pynormaliz to 1.7 to Upgrade normaliz to 3.4.0 and pynormaliz to 1.7+

Tested on Mac OS X. Works well except for interrupting (may require newer pynormaliz...) and the doctest failures reported in #23586 (fixing broken pynormaliz doctests).


New commits:

be0b9d2Upgrade normaliz, pynormaliz
a6ccc42normaliz: Back out 22684_cone_reduce_memory_usage.patch

comment:4 Changed 3 years ago by mkoeppe

  • Description modified (diff)

comment:5 Changed 3 years ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:6 Changed 3 years ago by git

  • Commit changed from a6ccc4235fa65a26fd772ad963df01b9ece5633f to 5e7a45b55eb85040b4a4c84fc0c8bd6506fdbdad

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

5e7a45bUpdate PyNormaliz to 1.8

comment:7 Changed 3 years ago by mkoeppe

  • Description modified (diff)
  • Summary changed from Upgrade normaliz to 3.4.0 and pynormaliz to 1.7+ to Upgrade normaliz to 3.4.0 and pynormaliz to 1.8

comment:8 Changed 2 years ago by mkoeppe

  • Description modified (diff)
  • Milestone changed from sage-8.1 to sage-8.2
  • Summary changed from Upgrade normaliz to 3.4.0 and pynormaliz to 1.8 to Upgrade normaliz to 3.5.1 and pynormaliz to 1.10

comment:9 Changed 2 years ago by git

  • Commit changed from 5e7a45b55eb85040b4a4c84fc0c8bd6506fdbdad to f2137cc5bb49278657ce23f0f8c775400b494023

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f6011b4Upgrade normaliz to 3.5.1 and pynormaliz to 1.10
5536407normaliz: Add flint dependency
71d2194normaliz: Back out 22684_cone_reduce_memory_usage.patch
f2137ccPolyhedron_normaliz: Adjust doctest to 'uniqueness in output' of normaliz 3.5.1

comment:10 Changed 2 years ago by mkoeppe

  • Cc vdelecroix tscrim jipilab added
  • Status changed from new to needs_review

comment:11 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:12 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to needs_work

I am now getting this crash (looks like an upstream problem):

sage: ieqs = [[0, 2, 0, -1, 0, 0, 0, 0, 0], [0, 0, 2, 0, -1, 0, 0, 0, 0],
....:         [1, -1, 0, 2, -1, 0, 0, 0, 0], [0, 0, -1, -1, 2, -1, 0, 0, 0],
....:         [0, 0, 0, 0, -1, 2, -1, 0, 0], [0, 0, 0, 0, 0, -1, 2, -1, 0],
....:         [0, 0, 0, 0, 0, 0, -1, 2, -1], [2, 0, 0, 0, 0, 0, 0, -1, 2],
....:         [0, -1, 0, 0, 0, 0, 0, 0, 0], [0, 0, -1, 0, 0, 0, 0, 0, 0],
....:         [0, 0, 0, -1, 0, 0, 0, 0, 0], [0, 0, 0, 0, -1, 0, 0, 0, 0],
....:         [0, 0, 0, 0, 0, -1, 0, 0, 0], [0, 0, 0, 0, 0, 0, -1, 0, 0],
....:         [0, 0, 0, 0, 0, 0, 0, -1, 0], [0, 0, 0, 0, 0, 0, 0, 0, -1],
....:         [-1, -1, -1, -1, -1, -1, -1, -1, -1], [3, 1, 0, 0, 0, 0, 0, 0, 0],
....:         [4, 0, 1, 0, 0, 0, 0, 0, 0], [6, 0, 0, 1, 0, 0, 0, 0, 0],
....:         [8, 0, 0, 0, 1, 0, 0, 0, 0], [6, 0, 0, 0, 0, 1, 0, 0, 0],
....:         [4, 0, 0, 0, 0, 0, 1, 0, 0], [2, 0, 0, 0, 0, 0, 0, 1, 0],
....:         [0, 0, 0, 0, 0, 0, 0, 0, 1]]
sage: P = Polyhedron(ieqs=ieqs, backend='normaliz')
sage: P.integral_points()
python2: ./libnormaliz/matrix.cpp:2901: libnormaliz::Matrix<Integer> libnormaliz::Matrix<Integer>::LLL_red(libnormaliz::Matrix<Integer>&, libnormaliz::Matrix<Integer>&) const [with Integer = double]: Assertion `(int) rank()==n' failed.
------------------------------------------------------------------------
/home/travis/sage-build/local/lib/python2.7/site-packages/cysignals/signals.so(+0x574b)[0x7fa5afdf274b]
/home/travis/sage-build/local/lib/python2.7/site-packages/cysignals/signals.so(+0x57b5)[0x7fa5afdf27b5]
/home/travis/sage-build/local/lib/python2.7/site-packages/cysignals/signals.so(+0x849a)[0x7fa5afdf549a]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x11390)[0x7fa5bcd37390]
/lib/x86_64-linux-gnu/libc.so.6(gsignal+0x38)[0x7fa5bc991428]
/lib/x86_64-linux-gnu/libc.so.6(abort+0x16a)[0x7fa5bc99302a]
/lib/x86_64-linux-gnu/libc.so.6(+0x2dbd7)[0x7fa5bc989bd7]
/lib/x86_64-linux-gnu/libc.so.6(+0x2dc82)[0x7fa5bc989c82]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZNK11libnormaliz6MatrixIdE7LLL_redERS1_S2_+0x5b9)[0x7fa47314f4a9]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZNK11libnormaliz6MatrixIdE17LLL_red_transposeERS1_S2_+0x83)[0x7fa47314fdc3]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZN11libnormaliz15LLL_coordinatesIxdEENS_25Sublattice_RepresentationIT_EERKNS_6MatrixIT0_EE+0x56)[0x7fa4730cfde6]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZN11libnormaliz31LLL_coordinates_without_1st_colIxxEEvRNS_25Sublattice_RepresentationIT_EENS_6MatrixIT0_EES7_b+0x291)[0x7fa4730d0881]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZN11libnormaliz14ProjectAndLiftIxxE7computeEbb+0x194)[0x7fa473104e54]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZN11libnormaliz4ConeI10__gmp_exprIA1_12__mpz_structS3_EE16project_and_liftERNS_14ConePropertiesERNS_6MatrixIS4_EERKS9_SA_b+0x839)[0x7fa473179b69]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZN11libnormaliz4ConeI10__gmp_exprIA1_12__mpz_structS3_EE31try_approximation_or_projectionERNS_14ConePropertiesE+0xf00)[0x7fa4731f5370]
/home/travis/sage-build/local/lib/libnormaliz.so.3(_ZN11libnormaliz4ConeI10__gmp_exprIA1_12__mpz_structS3_EE7computeENS_14ConePropertiesE+0x1e1)[0x7fa4731e8671]
/home/travis/sage-build/local/lib/python2.7/site-packages/PyNormaliz_cpp.so(_Z14_NmzResultImplI10__gmp_exprIA1_12__mpz_structS2_EEP7_objectPN11libnormaliz4ConeIT_EES5_+0x75)[0x7fa4734f6cc5]
/home/travis/sage-build/local/lib/python2.7/site-packages/PyNormaliz_cpp.so(_Z10_NmzResultP7_objectS0_+0x150)[0x7fa4734ed890]

Originally found by

sage: RC = RiggedConfigurations(['E',8,1], [[3,2]])
sage: len(RC.module_generators)
python2: ./libnormaliz/matrix.cpp:2901: libnormaliz::Matrix<Integer> libnormaliz::Matrix<Integer>::LLL_red(libnormaliz::Matrix<Integer>&, libnormaliz::Matrix<Integer>&) const [with Integer = double]: Assertion `(int) rank()==n' failed.

(Mathematical note, through trying to find a smaller example, I am noticing that E8(1) is special in that it is the only place where I see non-full-dimensional non-empty polytopes in my computations. Well, I also see a 0-dimensional polytope for type D4(3), but that doesn't cause a crash as it is likely special-cased.)


Also can you update the comment before --enable-flint to also say you enable flint as it is part of the standard packages of Sage?

comment:13 Changed 2 years ago by git

  • Commit changed from f2137cc5bb49278657ce23f0f8c775400b494023 to b8f7abc04085155225e54ad168ce4a7884fdd6cf

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

b8f7abcnormaliz/spkg-install: Explain --enable-flint

comment:14 Changed 2 years ago by mkoeppe

I can confirm this crash. Here on Mac OS X, this actually becomes an unhandled SIGABRT:

Assertion failed: ((int) rank()==n), function LLL_red, file ./libnormaliz/matrix.cpp, line 2901.
------------------------------------------------------------------------
0   signals.so                          0x000000010af572d8 print_backtrace + 40
1   ???                                 0x000000010dcac1e0 0x0 + 4526359008
------------------------------------------------------------------------
Unhandled SIGABRT: An abort() occurred.
This probably occurred because a *compiled* module has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Python will now terminate.

comment:15 Changed 2 years ago by mkoeppe

Upstream master branch (8db5b68491a105b9185a01c29c2f126f5115e238) has different behavior but does not fix this.

comment:16 Changed 2 years ago by tmonteil

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Thierry Monteil

I confirm that the packages build and pass self-tests and doctests correctly on both 64 and 32 bit architectures on Debian stretch. I also comfirm that there is a crash on both architectures regarding the test provided by Travis (which should be added in our doctests, once the issue is fixed).

comment:17 Changed 2 years ago by Winfried

I am not sure what the matrix ieqs represents. My interpretation: inhomogeneous inequalities, and we want to compute the lattice points in the polytope they define. Under this assumption I have made a Normaliz input file rash.in

amb_space auto
inhom_inequalities
[[0, 2, 0, -1, 0, 0, 0, 0, 0],
...
[0, 0, 0, 0, 0, 0, 0, 0, 1]]
HilbertBasis

Then a run of

normaliz -c crash.in

finishes without any problems in 3.5.1 and the intended 3.5.2. It turns out that the polytope is empty:

0 module generators
1252 Hilbert basis elements of recession monoid
0 vertices of polyhedron
39 extreme rays of recession cone
11 support hyperplanes of polyhedron (homogenized)

embedding dimension = 9
affine dimension of the polyhedron = -1
rank of recession monoid = 8
...

Is my interpretation of the input correct?

comment:18 Changed 2 years ago by Winfried

Of course, if the matrix represents inhomogeneous inequalities, what is the RHS? My input above assumes it is the last coordinate. I will now try with the first.

comment:19 Changed 2 years ago by Winfried

Unfortunately Normaliz crashes when coordinate 1 is interpreted as the RHS. Will take care of it.

comment:20 follow-up: Changed 2 years ago by Winfried

The problem is solved. Replace line 323 of source/libnormaliz/sublattice_representation.h

by

    if(Vertices.nr_of_rows()==0 || Vertices.rank()<EmbDim){ // use Supps for LLL coordinates

i.e., delete "-1" in this line.

comment:21 in reply to: ↑ 20 Changed 2 years ago by tscrim

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

Replying to Winfried:

The problem is solved. Replace line 323 of source/libnormaliz/sublattice_representation.h

by

    if(Vertices.nr_of_rows()==0 || Vertices.rank()<EmbDim){ // use Supps for LLL coordinates

i.e., delete "-1" in this line.

Great, thank you for figuring it out. I am guessing you are in the process of making a commit upstream or should I do a pull request?

comment:22 Changed 2 years ago by tscrim

  • Branch changed from u/mkoeppe/upgrade_normaliz_to_3_4_0_and_pynormaliz_to_1_7 to u/tscrim/upgrade_noramliz_pynormaliz-22984
  • Commit changed from b8f7abc04085155225e54ad168ce4a7884fdd6cf to f9f17a432b4d669ce88a963de93bbc2677b552be

I've pushed a branch with a fix for this problem. However, I have now come across another crash:

sage: RC = RiggedConfigurations(['E',8,1], [[4,2]])
sage: len(RC.module_generators)
python2: ./libnormaliz/project_and_lift.cpp:75: std::vector<long unsigned int> libnormaliz::ProjectAndLift<IntegerPL, IntegerRet>::order_supps(const libnormaliz::Matrix<Integer>&) [with IntegerPL = long long int; IntegerRet = long long int]: Assertion `Supps.nr_of_rows()>0' failed.
------------------------------------------------------------------------

I have verified that this did at least not crash before (well, up to my patience level of 2 minutes).

However, much more worrying is this:

sage: RC = RiggedConfigurations(['E',8,1], [[5,2]])
sage: %time len(RC.module_generators)
CPU times: user 25 s, sys: 52 ms, total: 25 s
Wall time: 6.95 s
6009

versus before

sage: RC = RiggedConfigurations(['E',8,1], [[5,2]])
sage: %time len(RC.module_generators)
CPU times: user 1min 11s, sys: 320 ms, total: 1min 11s
Wall time: 14.6 s
7066

Although the [5,1] example both returns 75.

I will extract the polytopes and find out which one gives a different number of lattice points. Give me a few minutes.


New commits:

f9f17a4Added patch to fix upstream issue #176.

comment:23 Changed 2 years ago by tscrim

Okay, here is the [4,1] computation to completion:

sage: RC = RiggedConfigurations(['E',8,1], [[4,2]])
sage: %time len(RC.module_generators)
CPU times: user 19min 37s, sys: 5.2 s, total: 19min 42s
Wall time: 4min 37s
288299

comment:24 Changed 2 years ago by tscrim

Okay, here is at least the first polytope that goes wrong:

sage: ieqs = [[0, 2, 0, -1, 0, 0, 0, 0, 0],
....:         [0, 0, 2, 0, -1, 0, 0, 0, 0],
....:         [1, -1, 0, 2, -1, 0, 0, 0, 0],
....:         [0, 0, -1, -1, 2, -1, 0, 0, 0],
....:         [0, 0, 0, 0, -1, 2, -1, 0, 0],
....:         [0, 0, 0, 0, 0, -1, 2, -1, 0],
....:         [1, 0, 0, 0, 0, 0, -1, 2, -1],
....:         [0, 0, 0, 0, 0, 0, 0, -1, 2],
....:         [0, -1, 0, 0, 0, 0, 0, 0, 0],
....:         [0, 0, -1, 0, 0, 0, 0, 0, 0],
....:         [0, 0, 0, -1, 0, 0, 0, 0, 0],
....:         [0, 0, 0, 0, -1, 0, 0, 0, 0],
....:         [0, 0, 0, 0, 0, -1, 0, 0, 0],
....:         [0, 0, 0, 0, 0, 0, -1, 0, 0],
....:         [0, 0, 0, 0, 0, 0, 0, -1, 0],
....:         [0, 0, 0, 0, 0, 0, 0, 0, -1],
....:         [-1, -1, -1, -1, -1, -1, -1, -1, -1],
....:         [2, 1, 0, 0, 0, 0, 0, 0, 0],
....:         [4, 0, 1, 0, 0, 0, 0, 0, 0],
....:         [4, 0, 0, 1, 0, 0, 0, 0, 0],
....:         [7, 0, 0, 0, 1, 0, 0, 0, 0],
....:         [6, 0, 0, 0, 0, 1, 0, 0, 0],
....:         [4, 0, 0, 0, 0, 0, 1, 0, 0],
....:         [2, 0, 0, 0, 0, 0, 0, 1, 0],
....:         [1, 0, 0, 0, 0, 0, 0, 0, 1]]
sage: P = Polyhedron(ieqs=ieqs, backend='normaliz')
sage: P.integral_points()
((-2, -2, -4, -5, -4, -3, -2, -1),
 (-2, -2, -4, -5, -4, -3, -2, 0),
 (-1, -2, -3, -4, -3, -2, -2, -1),
 (-1, -2, -3, -4, -3, -2, -1, 0),
 (-1, -1, -2, -2, -2, -2, -2, -1),
 (-1, -1, -2, -2, -1, -1, -1, 0),
 (-1, -1, -2, -2, -1, 0, 0, 0),
 (-1, 0, -2, -2, -2, -2, -2, -1),
 (0, -1, -1, -2, -2, -2, -2, -1),
 (0, 0, -1, -1, -1, -1, -1, 0))
sage: P
A 8-dimensional polyhedron in QQ^8 defined as the convex hull of 134 vertices

versus the new version with the patch:

sage: P.integral_points()
((-1, -2, -3, -4, -3, -2, -1, 0), (0, -1, -1, -2, -2, -2, -2, -1))

comment:25 Changed 2 years ago by tscrim

Here's also the example that causes the new crash:

sage: ieqs = [[0, 2, 0, -1, 0, 0, 0, 0, 0],
....:         [0, 0, 2, 0, -1, 0, 0, 0, 0],
....:         [0, -1, 0, 2, -1, 0, 0, 0, 0],
....:         [1, 0, -1, -1, 2, -1, 0, 0, 0],
....:         [0, 0, 0, 0, -1, 2, -1, 0, 0],
....:         [0, 0, 0, 0, 0, -1, 2, -1, 0],
....:         [1, 0, 0, 0, 0, 0, -1, 2, -1],
....:         [0, 0, 0, 0, 0, 0, 0, -1, 2],
....:         [0, -1, 0, 0, 0, 0, 0, 0, 0],
....:         [0, 0, -1, 0, 0, 0, 0, 0, 0],
....:         [0, 0, 0, -1, 0, 0, 0, 0, 0],
....:         [0, 0, 0, 0, -1, 0, 0, 0, 0],
....:         [0, 0, 0, 0, 0, -1, 0, 0, 0],
....:         [0, 0, 0, 0, 0, 0, -1, 0, 0],
....:         [0, 0, 0, 0, 0, 0, 0, -1, 0],
....:         [0, 0, 0, 0, 0, 0, 0, 0, -1],
....:         [-1, -1, -1, -1, -1, -1, -1, -1, -1],
....:         [3, 1, 0, 0, 0, 0, 0, 0, 0],
....:         [4, 0, 1, 0, 0, 0, 0, 0, 0],
....:         [6, 0, 0, 1, 0, 0, 0, 0, 0],
....:         [8, 0, 0, 0, 1, 0, 0, 0, 0],
....:         [6, 0, 0, 0, 0, 1, 0, 0, 0],
....:         [4, 0, 0, 0, 0, 0, 1, 0, 0],
....:         [2, 0, 0, 0, 0, 0, 0, 1, 0],
....:         [1, 0, 0, 0, 0, 0, 0, 0, 1]]
sage: P = Polyhedron(ieqs=ieqs, backend='normaliz')
sage: P.integral_points()
python2: ./libnormaliz/project_and_lift.cpp:75: std::vector<long unsigned int> libnormaliz::ProjectAndLift<IntegerPL, IntegerRet>::order_supps(const libnormaliz::Matrix<Integer>&) [with IntegerPL = long long int; IntegerRet = long long int]: Assertion `Supps.nr_of_rows()>0' failed.
------------------------------------------------------------------------

comment:26 Changed 2 years ago by mkoeppe

A hint: If you use P = Polyhedron(ieqs=ieqs, backend='normaliz', verbose=True), the PyNormaliz invocations are shown, making it easier for upstream

comment:27 Changed 2 years ago by tscrim

I wasn't aware of that, thank you. Although it does not yield much more information on the new crash:

sage: P = Polyhedron(ieqs=ieqs, backend='normaliz', verbose=True)
# Calling PyNormaliz.NmzCone(['inhom_equations', [], 'inhom_inequalities',
 [[2, 0, -1, 0, 0, 0, 0, 0, 0], [0, 2, 0, -1, 0, 0, 0, 0, 0],
 [-1, 0, 2, -1, 0, 0, 0, 0, 0], [0, -1, -1, 2, -1, 0, 0, 0, 1],
 [0, 0, 0, -1, 2, -1, 0, 0, 0], [0, 0, 0, 0, -1, 2, -1, 0, 0],
 [0, 0, 0, 0, 0, -1, 2, -1, 1], [0, 0, 0, 0, 0, 0, -1, 2, 0],
 [-1, 0, 0, 0, 0, 0, 0, 0, 0], [0, -1, 0, 0, 0, 0, 0, 0, 0],
 [0, 0, -1, 0, 0, 0, 0, 0, 0], [0, 0, 0, -1, 0, 0, 0, 0, 0],
 [0, 0, 0, 0, -1, 0, 0, 0, 0], [0, 0, 0, 0, 0, -1, 0, 0, 0],
 [0, 0, 0, 0, 0, 0, -1, 0, 0], [0, 0, 0, 0, 0, 0, 0, -1, 0],
 [-1, -1, -1, -1, -1, -1, -1, -1, -1], [1, 0, 0, 0, 0, 0, 0, 0, 3],
 [0, 1, 0, 0, 0, 0, 0, 0, 4], [0, 0, 1, 0, 0, 0, 0, 0, 6],
 [0, 0, 0, 1, 0, 0, 0, 0, 8], [0, 0, 0, 0, 1, 0, 0, 0, 6],
 [0, 0, 0, 0, 0, 1, 0, 0, 4], [0, 0, 0, 0, 0, 0, 1, 0, 2],
 [0, 0, 0, 0, 0, 0, 0, 1, 1]]])
sage: P.integral_points()
python2: ./libnormaliz/project_and_lift.cpp:75: std::vector<long unsigned int> libnormaliz::ProjectAndLift<IntegerPL, IntegerRet>::order_supps(const libnormaliz::Matrix<Integer>&) [with IntegerPL = long long int; IntegerRet = long long int]: Assertion `Supps.nr_of_rows()>0' failed.
------------------------------------------------------------------------

Here is also the output for the lattice point computation:

sage: P = Polyhedron(ieqs=ieqs, backend='normaliz', verbose=True)
# Calling PyNormaliz.NmzCone(['inhom_equations', [], 'inhom_inequalities',
 [[2, 0, -1, 0, 0, 0, 0, 0, 0], [0, 2, 0, -1, 0, 0, 0, 0, 0],
 [-1, 0, 2, -1, 0, 0, 0, 0, 1], [0, -1, -1, 2, -1, 0, 0, 0, 0],
 [0, 0, 0, -1, 2, -1, 0, 0, 0], [0, 0, 0, 0, -1, 2, -1, 0, 0],
 [0, 0, 0, 0, 0, -1, 2, -1, 1], [0, 0, 0, 0, 0, 0, -1, 2, 0],
 [-1, 0, 0, 0, 0, 0, 0, 0, 0], [0, -1, 0, 0, 0, 0, 0, 0, 0],
 [0, 0, -1, 0, 0, 0, 0, 0, 0], [0, 0, 0, -1, 0, 0, 0, 0, 0],
 [0, 0, 0, 0, -1, 0, 0, 0, 0], [0, 0, 0, 0, 0, -1, 0, 0, 0],
 [0, 0, 0, 0, 0, 0, -1, 0, 0], [0, 0, 0, 0, 0, 0, 0, -1, 0],
 [-1, -1, -1, -1, -1, -1, -1, -1, -1], [1, 0, 0, 0, 0, 0, 0, 0, 2],
 [0, 1, 0, 0, 0, 0, 0, 0, 4], [0, 0, 1, 0, 0, 0, 0, 0, 4],
 [0, 0, 0, 1, 0, 0, 0, 0, 7], [0, 0, 0, 0, 1, 0, 0, 0, 6],
 [0, 0, 0, 0, 0, 1, 0, 0, 4], [0, 0, 0, 0, 0, 0, 1, 0, 2],
 [0, 0, 0, 0, 0, 0, 0, 1, 1]]])
sage: P.integral_points()
((-1, -2, -3, -4, -3, -2, -1, 0), (0, -1, -1, -2, -2, -2, -2, -1))
Last edited 2 years ago by tscrim (previous) (diff)

comment:28 Changed 2 years ago by tscrim

PS - That means the docstring for Polyhedron needs a minor adjustment:

    - ``verbose`` -- boolean (default: ``False``). Whether to print
      verbose output for debugging purposes. Only supported by the cdd
      backends.

comment:29 follow-up: Changed 2 years ago by Winfried

The wrong number of lattice points is a more severe problem. I don't have an explanation for it.

The (first and solved) problem in sublattice_representation.h has nothing to do with it. After the change in sublattice_representation.h:323 a second problem has become visible there in Normaliz' own tests. One must change line 325 to

        if(SuppHelp.rank()<EmbDim-1)

But againthis has nothing to do with the wrong number of lattice points.

comment:30 Changed 2 years ago by git

  • Commit changed from f9f17a432b4d669ce88a963de93bbc2677b552be to d3f7ceb711bc98ae06bf4d9a921ee376c09c3fda

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

d3f7cebNew version of patch to fix next crash.

comment:31 in reply to: ↑ 29 Changed 2 years ago by tscrim

Replying to Winfried:

The wrong number of lattice points is a more severe problem. I don't have an explanation for it.

I am afraid all I can do is give you examples from my use-case as I do not understand too much about how Normaliz works. Thank you for your continued efforts and quick responses.

The (first and solved) problem in sublattice_representation.h has nothing to do with it. After the change in sublattice_representation.h:323 a second problem has become visible there in Normaliz' own tests. One must change line 325 to

        if(SuppHelp.rank()<EmbDim-1)

But again this has nothing to do with the wrong number of lattice points.

I added that to the patch, but (as you might be aware) I am now getting yet another crash with the same example as in comment:25:

sage: P = Polyhedron(ieqs=ieqs, backend='normaliz', verbose=True)
# Calling PyNormaliz.NmzCone(['inhom_equations', [], 'inhom_inequalities',
 [[2, 0, -1, 0, 0, 0, 0, 0, 0], [0, 2, 0, -1, 0, 0, 0, 0, 0],
 [-1, 0, 2, -1, 0, 0, 0, 0, 0], [0, -1, -1, 2, -1, 0, 0, 0, 1],
 [0, 0, 0, -1, 2, -1, 0, 0, 0], [0, 0, 0, 0, -1, 2, -1, 0, 0],
 [0, 0, 0, 0, 0, -1, 2, -1, 1], [0, 0, 0, 0, 0, 0, -1, 2, 0],
 [-1, 0, 0, 0, 0, 0, 0, 0, 0], [0, -1, 0, 0, 0, 0, 0, 0, 0],
 [0, 0, -1, 0, 0, 0, 0, 0, 0], [0, 0, 0, -1, 0, 0, 0, 0, 0],
 [0, 0, 0, 0, -1, 0, 0, 0, 0], [0, 0, 0, 0, 0, -1, 0, 0, 0],
 [0, 0, 0, 0, 0, 0, -1, 0, 0], [0, 0, 0, 0, 0, 0, 0, -1, 0],
 [-1, -1, -1, -1, -1, -1, -1, -1, -1], [1, 0, 0, 0, 0, 0, 0, 0, 3],
 [0, 1, 0, 0, 0, 0, 0, 0, 4], [0, 0, 1, 0, 0, 0, 0, 0, 6],
 [0, 0, 0, 1, 0, 0, 0, 0, 8], [0, 0, 0, 0, 1, 0, 0, 0, 6],
 [0, 0, 0, 0, 0, 1, 0, 0, 4], [0, 0, 0, 0, 0, 0, 1, 0, 2],
 [0, 0, 0, 0, 0, 0, 0, 1, 1]]])
sage: P
A 8-dimensional polyhedron in QQ^8 defined as the convex hull of 132 vertices
sage: P.integral_points()
python2: ./libnormaliz/project_and_lift.cpp:75: std::vector<long unsigned int> libnormaliz::ProjectAndLift<IntegerPL, IntegerRet>::order_supps(const libnormaliz::Matrix<Integer>&) [with IntegerPL = long long int; IntegerRet = long long int]: Assertion `Supps.nr_of_rows()>0' failed.
------------------------------------------------------------------------

Addendum - It is the same error as before.

Last edited 2 years ago by tscrim (previous) (diff)

comment:32 Changed 2 years ago by Winfried

I think all the observed problems are solved. More tomorrow.

comment:33 Changed 2 years ago by tscrim

Great, thank you!

comment:34 Changed 2 years ago by Winfried

The problem with the wrong number of lattice points (or a failing assertion) was not caused by a severe problem, but by a silly bug. As a temporary fix I suggest to include as lines cone.cpp:3804 and 3805

    if(!is_parallelotope)
        Pair.clear();

So with some surrounding the code there should be

            is_Computed.set(ConeProperty::IsPointed);
        }
    }
    
    if(!is_parallelotope)
        Pair.clear();
    
    if(!inhomogeneous && !isComputed(ConeProperty::Grading))
        return;

I plan to release 3.5.2 at the end of this week. It will contain a more conceptual solution.

Please test.

comment:35 Changed 2 years ago by git

  • Commit changed from d3f7ceb711bc98ae06bf4d9a921ee376c09c3fda to 737552d95ff6d810c56611560fa9b62f888cdafc

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

737552dUpdating patch with upstream fix for wrong number of lattice points.

comment:36 follow-up: Changed 2 years ago by tscrim

Thank you. I have verified that I get again

sage: RC = RiggedConfigurations(['E',8,1], [[5,2]])
sage: %time len(RC.module_generators)
CPU times: user 27.6 s, sys: 68 ms, total: 27.7 s
Wall time: 7.8 s
7066
sage: RC = RiggedConfigurations(['E',8,1], [[4,2]])
sage: %time len(RC.module_generators)
CPU times: user 5min 6s, sys: 944 ms, total: 5min 7s
Wall time: 1min 36s
288299

and the >2x speedup is very impressive. :) I've updated the patch accordingly.

@mkoeppe Perhaps we should just wait on this upgrade until 3.5.2 is released to avoid the patch since it will be in just a few days?

As far as adding tests go, I think we should explicitly add the polytopes mentioned above rather than the RC code. They are isolated and much faster to test.

comment:37 in reply to: ↑ 36 Changed 2 years ago by mkoeppe

Replying to tscrim:

@mkoeppe Perhaps we should just wait on this upgrade until 3.5.2 is released to avoid the patch since it will be in just a few days?

Yes, I think so too.

As far as adding tests go, I think we should explicitly add the polytopes mentioned above rather than the RC code. They are isolated and much faster to test.

I agree.

comment:38 Changed 2 years ago by git

  • Commit changed from 737552d95ff6d810c56611560fa9b62f888cdafc to 57d40b030deb164404f6a245af0c3734ebf1eb40

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

530302dMerge branch 'u/tscrim/upgrade_noramliz_pynormaliz-22984' of git://trac.sagemath.org/sage into u/tscrim/upgrade_noramliz_pynormaliz-22984
57d40b0Upgrade Normaliz to 3.5.2.

comment:39 Changed 2 years ago by tscrim

  • Description modified (diff)
  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Completely fixed; Fix reported upstream
  • Status changed from needs_work to needs_info

Here is with 3.5.2 and the tests I did above now work. Where would a good place be to add the doctests?

comment:40 Changed 2 years ago by mkoeppe

  • Status changed from needs_info to needs_work
  • Summary changed from Upgrade normaliz to 3.5.1 and pynormaliz to 1.10 to Upgrade normaliz to 3.5.2 and pynormaliz to 1.10

I would think the doctests should go to Polyhedron_normaliz.integral_points.

comment:41 Changed 2 years ago by git

  • Commit changed from 57d40b030deb164404f6a245af0c3734ebf1eb40 to 6ba442d0586b9fdedec07a2e8f50bcd5d1a4a913

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

6ba442dAdding tests from comment:24,25 of #22984.

comment:42 Changed 2 years ago by tscrim

  • Status changed from needs_work to needs_review

Okay, done.

comment:43 Changed 2 years ago by tscrim

ping

comment:44 Changed 2 years ago by mkoeppe

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Upgrade normaliz to 3.5.2 and pynormaliz to 1.10 to Upgrade normaliz to 3.5.2 and pynormaliz to 1.12
  1. Gutsche informs us that we should upgrade pynormaliz to 1.12

comment:45 Changed 2 years ago by mkoeppe

  • Cc gh-sebasguts added

comment:46 follow-up: Changed 2 years ago by jipilab

  • Reviewers changed from Travis Scrimshaw, Thierry Monteil to Travis Scrimshaw, Thierry Monteil, Jean-Philippe Labbé

All tests passed with sage8.2.beta8.

@Travis: Do you still want to add a comment about the verbose of Polyhedron?

Otherwise, it seems that we need to wait for 1.12 of pynormaliz check everything one last time.

comment:47 Changed 2 years ago by mkoeppe

  • Branch changed from u/tscrim/upgrade_noramliz_pynormaliz-22984 to u/mkoeppe/upgrade_noramliz_pynormaliz-22984

comment:48 Changed 2 years ago by mkoeppe

  • Commit changed from 6ba442d0586b9fdedec07a2e8f50bcd5d1a4a913 to 183087a21eaaf64b464fa6ffe0aaf94819d6626c
  • Status changed from needs_work to needs_review

New commits:

183087aUpgrade PyNormaliz to 1.12

comment:49 Changed 2 years ago by jipilab

  • Branch changed from u/mkoeppe/upgrade_noramliz_pynormaliz-22984 to u/jipilab/upgrade_normaliz_pynormaliz-22984
  • Commit changed from 183087a21eaaf64b464fa6ffe0aaf94819d6626c to 7f47ec9f71498f744614e28ed8316b72b1904bdd
  • Status changed from needs_review to positive_review

New commits:

7f47ec9Adapted the polyhedron docstring

comment:50 Changed 2 years ago by jipilab

It looks good to me now and all tests pass in the polyhedron folder.

comment:51 Changed 2 years ago by mkoeppe

  • Status changed from positive_review to needs_work

Compilation errors with a specific gcc version with PyNormaliz?. Fixed in normaliz 3.5.3

comment:52 Changed 2 years ago by mkoeppe

  • Description modified (diff)
  • Summary changed from Upgrade normaliz to 3.5.2 and pynormaliz to 1.12 to Upgrade normaliz to 3.5.3 and pynormaliz to 1.12

comment:53 Changed 2 years ago by mkoeppe

  • Branch changed from u/jipilab/upgrade_normaliz_pynormaliz-22984 to u/mkoeppe/upgrade_normaliz_pynormaliz-22984

comment:54 Changed 2 years ago by mkoeppe

  • Commit changed from 7f47ec9f71498f744614e28ed8316b72b1904bdd to 117e428c112921f5a1d555daf3a7b0ed10828532
  • Status changed from needs_work to needs_review

New commits:

117e428Upgrade normaliz to 3.5.3

comment:55 Changed 2 years ago by jipilab

Install the latest normaliz and got:

~/sage/src/sage/geometry/polyhedron$ sage -t *.py
Using --optional=4ti2,bliss,latte_int,lidia,lrslib,mpir,normaliz,pynormaliz,python2,sage
Doctesting 25 files.
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

comment:56 Changed 2 years ago by jipilab

  • Status changed from needs_review to positive_review

comment:57 in reply to: ↑ 46 ; follow-up: Changed 2 years ago by tscrim

Replying to jipilab:

@Travis: Do you still want to add a comment about the verbose of Polyhedron?

It can be on a separate ticket, but the docstring for verbose should also say it is supported for the normaliz backend as well.

comment:58 in reply to: ↑ 57 Changed 2 years ago by jipilab

Replying to tscrim:

Replying to jipilab:

@Travis: Do you still want to add a comment about the verbose of Polyhedron?

It can be on a separate ticket, but the docstring for verbose should also say it is supported for the normaliz backend as well.

I went ahead and made the change already... I realized it was not a big deal.

comment:59 Changed 2 years ago by tscrim

Ah, thanks. I didn't look at the diffs.

comment:60 Changed 2 years ago by jipilab

  • Branch changed from u/mkoeppe/upgrade_normaliz_pynormaliz-22984 to u/jipilab/upgrade_normaliz_pynormaliz-22984
  • Commit changed from 117e428c112921f5a1d555daf3a7b0ed10828532 to 7057ef457e5e6f78197100f938b8dae8d8b0c5a4

Merged the forgotten commit.


New commits:

7f47ec9Adapted the polyhedron docstring
7057ef4Merge branch to get docstring adaptation

comment:61 Changed 2 years ago by jipilab

  • Status changed from positive_review to needs_review

comment:62 Changed 2 years ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:63 Changed 2 years ago by mkoeppe

  • Keywords IMA-PolyGeom added

comment:64 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:65 Changed 2 years ago by jipilab

  • Milestone changed from sage-8.2 to sage-8.3

comment:66 Changed 2 years ago by vbraun

  • Branch changed from u/jipilab/upgrade_normaliz_pynormaliz-22984 to 7057ef457e5e6f78197100f938b8dae8d8b0c5a4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.