Opened 3 months ago
Last modified 4 weeks ago
#31245 needs_review enhancement
Implement parallel fvector for polytopes
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  geometry  Keywords:  parallel fvector 
Cc:  jipilab, ghLaisRast, stumpc5, tscrim  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  u/ghkliem/first_parallel_version_of_face_iterator_reb (Commits, GitHub, GitLab)  Commit:  ae482a4d07ebc5f68fe78b37a32919c85b94aef5 
Dependencies:  #31474, #31499  Stopgaps: 
Description (last modified by )
This ticket parallelizes the fvector for polytopes.
Each thread has its private structure with which it does partial jobs. Depending on the parallelization depth, there is one job per face of fixed codimension (usually 1,2 or 3). After everything has finished, the partial fvectors will be added.
Actually, every face is visited and thus the code could be modified in the future, to explore other properties of faces then just the dimension. The parallelization seems to work well with at least 40 threads (for computations taking long enough such that this pays off, see https://arxiv.org/pdf/1905.01945.pdf).
Also the algorithm does work in other situations (simplicial complex, lattice of flats of a matroid) and this parallel structure could be used for this as well.
On the downside, sig_on()/sig_off()
doesn't work with with multiple threads and has to be replaced by a simple sig_check()
. Also raising errors in parallel code results in terrible slow down. Hence the errors are replaced by returning the exception value. In case of a bug there won't be any traceback anymore, but at least also no segmenation fault.
Before:
sage: P = P.base_extend(QQ) sage: P = P.base_extend(QQ) sage: Q = P.join(P.polar(in_affine_span=True)) sage: C = CombinatorialPolyhedron(Q) sage: %time C.f_vector() CPU times: user 111 ms, sys: 186 µs, total: 111 ms Wall time: 111 ms (1, 150, 3990, 25590, 69450, 95402, 69450, 25590, 3990, 150, 1) sage: P = polytopes.Birkhoff_polytope(5) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector() CPU times: user 584 ms, sys: 25 µs, total: 584 ms Wall time: 583 ms (1, 120, 5040, 50250, 233400, 631700, 1113700, 1367040, 1220550, 817150, 419225, 167200, 52120, 12600, 2300, 300, 25, 1) # Using the <simple> version of the algorithm. sage: P = polytopes.associahedron(['A', 11], backend='normaliz') sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector() CPU times: user 37.9 s, sys: 17.2 ms, total: 37.9 s Wall time: 37.9 s (1, 208012, 1144066, 2735810, 3730650, 3197700, 1790712, 659736, 157080, 23100, 1925, 77, 1)
After (machine has 4 cores):
sage: P = polytopes.permutahedron(5) sage: P = P.base_extend(QQ) sage: Q = P.join(P.polar(in_affine_span=True)) sage: C = CombinatorialPolyhedron(Q) sage: %time C.f_vector(num_threads=1) CPU times: user 107 ms, sys: 0 ns, total: 107 ms Wall time: 107 ms (1, 150, 3990, 25590, 69450, 95402, 69450, 25590, 3990, 150, 1) sage: C = CombinatorialPolyhedron(Q) sage: %time C.f_vector(num_threads=2) CPU times: user 108 ms, sys: 0 ns, total: 108 ms Wall time: 55.6 ms (1, 150, 3990, 25590, 69450, 95402, 69450, 25590, 3990, 150, 1) sage: C = CombinatorialPolyhedron(Q) sage: %time C.f_vector(num_threads=4) CPU times: user 147 ms, sys: 52 µs, total: 147 ms Wall time: 38.6 ms (1, 150, 3990, 25590, 69450, 95402, 69450, 25590, 3990, 150, 1) sage: C = CombinatorialPolyhedron(Q) sage: %time C.f_vector(num_threads=8) CPU times: user 236 ms, sys: 0 ns, total: 236 ms Wall time: 31.2 ms (1, 150, 3990, 25590, 69450, 95402, 69450, 25590, 3990, 150, 1) sage: P = polytopes.Birkhoff_polytope(5) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=1) CPU times: user 354 ms, sys: 0 ns, total: 354 ms Wall time: 354 ms (1, 120, 5040, 50250, 233400, 631700, 1113700, 1367040, 1220550, 817150, 419225, 167200, 52120, 12600, 2300, 300, 25, 1) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=2) CPU times: user 363 ms, sys: 0 ns, total: 363 ms Wall time: 181 ms (1, 120, 5040, 50250, 233400, 631700, 1113700, 1367040, 1220550, 817150, 419225, 167200, 52120, 12600, 2300, 300, 25, 1) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=4) CPU times: user 459 ms, sys: 0 ns, total: 459 ms Wall time: 117 ms (1, 120, 5040, 50250, 233400, 631700, 1113700, 1367040, 1220550, 817150, 419225, 167200, 52120, 12600, 2300, 300, 25, 1) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=8) CPU times: user 776 ms, sys: 154 µs, total: 776 ms Wall time: 103 ms (1, 120, 5040, 50250, 233400, 631700, 1113700, 1367040, 1220550, 817150, 419225, 167200, 52120, 12600, 2300, 300, 25, 1) # Using the <simple> version of the algorithm. sage: P = polytopes.associahedron(['A', 11], backend='normaliz') sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=1) CPU times: user 33.5 s, sys: 0 ns, total: 33.5 s Wall time: 33.5 s (1, 208012, 1144066, 2735810, 3730650, 3197700, 1790712, 659736, 157080, 23100, 1925, 77, 1) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=2) CPU times: user 34.4 s, sys: 3.49 ms, total: 34.4 s Wall time: 17.2 s (1, 208012, 1144066, 2735810, 3730650, 3197700, 1790712, 659736, 157080, 23100, 1925, 77, 1) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=4) CPU times: user 35.9 s, sys: 15.5 ms, total: 35.9 s Wall time: 9 s (1, 208012, 1144066, 2735810, 3730650, 3197700, 1790712, 659736, 157080, 23100, 1925, 77, 1) sage: C = CombinatorialPolyhedron(P) sage: %time C.f_vector(num_threads=8) CPU times: user 1min 6s, sys: 31.3 ms, total: 1min 6s Wall time: 8.44 s (1, 208012, 1144066, 2735810, 3730650, 3197700, 1790712, 659736, 157080, 23100, 1925, 77, 1)
Change History (31)
comment:1 Changed 3 months ago by
 Status changed from new to needs_review
comment:2 Changed 3 months ago by
 Description modified (diff)
comment:3 followup: ↓ 7 Changed 3 months ago by
 Reviewers set to Travis Scrimshaw
comment:4 Changed 3 months ago by
 Commit changed from d051f283c08d117bd51d2167fb3d0ccf641dd620 to 0dae7a061df9a2f05f9ef2d43602cc04fb351fb0
Branch pushed to git repo; I updated commit sha1. New commits:
0dae7a0  remove redundant sig_checks

comment:5 Changed 3 months ago by
 Commit changed from 0dae7a061df9a2f05f9ef2d43602cc04fb351fb0 to e3bdc617538af4ab8cdcf147fd74f4e62da5ff2a
comment:6 Changed 3 months ago by
 Dependencies set to #31226
comment:7 in reply to: ↑ 3 Changed 3 months ago by
Replying to tscrim:
The
sig_check()
outside the loop is unnecessary:cdef size_t j + sig_check() for j in range(n_faces): + sig_check() if (is_not_maximal_fused(new_faces, j, algorithm) or # Step 2 is_contained_in_one_fused(new_faces.faces[j], visited_all, algorithm)): # Step 3 is_not_new_face[j] = TrueLikewise here:
face_list_t visited_all) nogil except 1: cdef size_t output  sig_on() + sig_check() if faces.polyhedron_is_simple: output = get_next_level_fused(faces, new_faces, visited_all, <simple> 0) else: output = get_next_level_fused(faces, new_faces, visited_all, <standard> 0)  sig_off() return output cdef inline size_t bit_rep_to_coatom_rep(face_t face, face_list_t coatoms, size_t *output):Here you either need the
sig_on/sig_off
or some more targeted checking. I don't see the point in performing a single check here.Otherwise LGTM.
I removed the redundant sig_checks.
As mentioned, sig_on, sig_off doesn't work well with parallelization. It crashes hard because sig_on, sig_off doesn't appear in correct order anymore.
Wrapping prange
in sig_on/sig_off has a large performance penalty (8.45 s > 9 s).
Currently there is one sig_check
before checking that a face is inclusion maximal. One can add more sig checks, which wouldn't affect performance much for large examples, but for the small examples there is a penalty of about 50 percent.
I think the current solution should be fine: The largest thing I ever computed with this algorithm, which took about a month on 40 cores, had 11,665,781 vertices and 162 facets. In this case between the sig_checks A &~ B == 0
is performed for about 2 billion bits. Currently, without intrinsics, I get:
sage: B = Bitset((randint(0,2**20) for _ in range(2**19))) sage: %timeit B.issubset(B) 6.18 µs ± 9.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) sage: %timeit B.intersection_update(B) 6.02 µs ± 2.72 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
On this rate performing this for 2 billion bits should take about 12 ms. And even if the example has many more coatoms, in the worst case (completely unrealistic, because the dimension is not 1 and more memory is needed for other dimensions and probably for other threads), there are bitwise operations performed twice for the entire RAM. Say, we have 1 TB RAM, this would then take 96 seconds. Okay, that is long, but in any realistic sceanrio it would only take about a second, even with 1 TB RAM.
To sum up, I think the current amount of sig_checks
suffices for the next years until more than 1 TB RAM is common (and even then, it doubt that there is an application for such a large example, as it would take forever to compute).
comment:8 Changed 3 months ago by
 Branch changed from u/ghkliem/first_parallel_version_of_face_iterator to u/ghkliem/first_parallel_version_of_face_iterator_reb
 Commit changed from e3bdc617538af4ab8cdcf147fd74f4e62da5ff2a to 928ea608dc56698cd8df3d71b8266e75c28670eb
Rebased.
I didn't have this ticket here ready when writing #30445.
Eventually, one could also store edges/ridges or any sort of face in parallel. This would also help with applications for simplicial complexes and lattice of flats of manifolds.
But that should be left for another time.
New commits:
6a2a9b4  fix merge conflict with develop in particual #30445

928ea60  remove redundant allocation

comment:9 Changed 3 months ago by
 Status changed from needs_review to positive_review
Thank you. This is quite a nice improvement.
comment:10 Changed 3 months ago by
Thank you.
comment:11 Changed 2 months ago by
On the kucalc buildbot:
[sagelib9.3.beta6] [1/1] gcc Wnounusedresult Wsigncompare DNDEBUG g fwrapv O3 Wall Wnounused O2 g march=native fPIC I/var/lib/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/cysignals I./sage/data_structures I./sage/cpython Isage/cpython Isage/data_structures I/var/lib/buildbot/slave/sage_git/build/build/pkgs/sagelib/src I/var/lib/buildbot/slave/sage_git/build/local/include/python3.9 I/var/lib/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/numpy/core/include Ibuild/cythonized I/var/lib/buildbot/slave/sage_git/build/local/include/python3.9 c build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c o build/temp.linuxx86_643.9/build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.o fnostrictaliasing DCYTHON_CLINE_IN_TRACEBACK=1 fopenmp std=c99 [sagelib9.3.beta6] In file included from build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:660:0: [sagelib9.3.beta6] /var/lib/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/cysignals/struct_signals.h:45:1: sorry, unimplemented: ‘_Atomic’ with OpenMP [sagelib9.3.beta6] typedef volatile _Atomic int cy_atomic_int; [sagelib9.3.beta6] ^ [sagelib9.3.beta6] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c: In function ‘__pyx_f_4sage_8geometry_10polyhedron_24combinatorial_polyhedron_13face_iterator_prepare_face_iterator_for_partial_job’: [sagelib9.3.beta6] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9035:54: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta6] __pyx_t_2 = ((__pyx_v_structure>current_dimension == (__pyx_v_structure>dimension  __pyx_v_parallelization_depth)) != 0); [sagelib9.3.beta6] ^ [sagelib9.3.beta6] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9322:84: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta6] __pyx_t_1 = (((__pyx_v_parallel_struct>current_job_id[__pyx_v_current_depth]) == 1L) != 0); [sagelib9.3.beta6] ^ [sagelib9.3.beta6] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9673:54: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta6] __pyx_t_1 = ((__pyx_v_structure>current_dimension != ((__pyx_v_structure>dimension  __pyx_v_parallelization_depth)  1)) != 0); [sagelib9.3.beta6] ^ [sagelib9.3.beta6] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c: In function ‘__Pyx_InitGlobals’: [sagelib9.3.beta6] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:23283:1: warning: ‘PyEval_InitThreads’ is deprecated [Wdeprecateddeclarations] [sagelib9.3.beta6] PyEval_InitThreads(); [sagelib9.3.beta6] ^ [sagelib9.3.beta6] In file included from /var/lib/buildbot/slave/sage_git/build/local/include/python3.9/Python.h:145:0, [sagelib9.3.beta6] from build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:52: [sagelib9.3.beta6] /var/lib/buildbot/slave/sage_git/build/local/include/python3.9/ceval.h:130:37: note: declared here [sagelib9.3.beta6] Py_DEPRECATED(3.9) PyAPI_FUNC(void) PyEval_InitThreads(void); [sagelib9.3.beta6] ^ [sagelib9.3.beta6] error: command '/usr/bin/gcc' failed with exit code 1
comment:12 Changed 2 months ago by
 Status changed from positive_review to needs_work
comment:13 Changed 2 months ago by
This appears to be an issue with python 3.5 to 3.7 according to https://bugs.python.org/issue25150
From this I get that this can be "fixed" by doing this specific module as a C++ extension (or by requiring system python >= 3.8, but that isn't an option for the moment).
I'll check if I can reproduce this with system python 3.7 (need to rebuild first).
comment:14 Changed 2 months ago by
Actually, that is a problem with cysignals that should be fixed there.
comment:15 Changed 2 months ago by
 Commit changed from 928ea608dc56698cd8df3d71b8266e75c28670eb to baf80deaca74b037c11e5eee914009e216f3beed
Branch pushed to git repo; I updated commit sha1. New commits:
baf80de  try disabling CYSIGNALS_C_ATOMIC

comment:16 Changed 2 months ago by
According to
https://github.com/sagemath/cysignals/blob/master/src/cysignals/struct_signals.h
and
https://github.com/sagemath/cysignals/blob/master/src/cysignals/cysignals_config.h.in
just undefininig CYSIGNALS_C_ATOMIC
should fix this.
comment:17 Changed 2 months ago by
 Status changed from needs_work to needs_review
comment:18 Changed 2 months ago by
 Status changed from needs_review to positive_review
Back off to the builbots.
comment:19 followup: ↓ 24 Changed 5 weeks ago by
Build fails on various buildbots:
[sagelib9.3.beta7] [1/1] gcc Wnounusedresult Wsigncompare DNDEBUG g fwrapv O3 Wall Wnounused O2 g march=native O2 g march=native fPIC UCYSIGNALS_C_ATOMIC I/home/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/cysignals I./sage/data_structures I./sage/cpython Isage/cpython I/home/buildbot/slave/sage_git/build/build/pkgs/sagelib/src I/home/buildbot/slave/sage_git/build/local/include/python3.9 I/home/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/numpy/core/include Ibuild/cythonized I/home/buildbot/slave/sage_git/build/local/include/python3.9 c build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c o build/temp.linuxx86_643.9/build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.o fnostrictaliasing DCYTHON_CLINE_IN_TRACEBACK=1 fopenmp std=c99 [sagelib9.3.beta7] In file included from build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:662:0: [sagelib9.3.beta7] /home/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/cysignals/struct_signals.h:45:1: sorry, unimplemented: ‘_Atomic’ with OpenMP [sagelib9.3.beta7] typedef volatile _Atomic int cy_atomic_int; [sagelib9.3.beta7] ^~~~~~~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c: In function ‘__pyx_f_4sage_8geometry_10polyhedron_24combinatorial_polyhedron_13face_iterator_prepare_face_iterator_for_partial_job’: [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9037:54: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta7] __pyx_t_2 = ((__pyx_v_structure>current_dimension == (__pyx_v_structure>dimension  __pyx_v_parallelization_depth)) != 0); [sagelib9.3.beta7] ^~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9324:84: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta7] __pyx_t_1 = (((__pyx_v_parallel_struct>current_job_id[__pyx_v_current_depth]) == 1L) != 0); [sagelib9.3.beta7] ^~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9675:54: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta7] __pyx_t_1 = ((__pyx_v_structure>current_dimension != ((__pyx_v_structure>dimension  __pyx_v_parallelization_depth)  1)) != 0); [sagelib9.3.beta7] ^~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c: In function ‘__Pyx_InitGlobals’: [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:23285:1: warning: ‘PyEval_InitThreads’ is deprecated [Wdeprecateddeclarations] [sagelib9.3.beta7] PyEval_InitThreads(); [sagelib9.3.beta7] ^~~~~~~~~~~~~~~~~~ [sagelib9.3.beta7] In file included from /home/buildbot/slave/sage_git/build/local/include/python3.9/Python.h:145:0, [sagelib9.3.beta7] from build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:54: [sagelib9.3.beta7] /home/buildbot/slave/sage_git/build/local/include/python3.9/ceval.h:130:37: note: declared here [sagelib9.3.beta7] Py_DEPRECATED(3.9) PyAPI_FUNC(void) PyEval_InitThreads(void); [sagelib9.3.beta7] ^~~~~~~~~~~~~~~~~~ [sagelib9.3.beta7] error: command '/usr/bin/gcc' failed with exit code 1 [sagelib9.3.beta7] [sagelib9.3.beta7] real 0m3.325s [sagelib9.3.beta7] user 0m2.798s [sagelib9.3.beta7] sys 0m0.563s Makefile:2233: recipe for target 'sagelibnodeps' failed
comment:20 Changed 5 weeks ago by
 Commit changed from baf80deaca74b037c11e5eee914009e216f3beed to da65d28f81ddb7a865d0af830d14bb4132c3c1cb
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
da65d28  Revert "try disabling CYSIGNALS_C_ATOMIC"

comment:21 Changed 5 weeks ago by
 Dependencies changed from #31226 to #31226, #31455
comment:22 Changed 5 weeks ago by
 Milestone changed from sage9.3 to sage9.4
I don't think this is going to happen with 9.3 anymore, if this is waiting on a cysignals patch/update.
comment:23 Changed 5 weeks ago by
 Commit changed from da65d28f81ddb7a865d0af830d14bb4132c3c1cb to 405ef3e3bb65ac1c610727c590daf75c19048c45
Branch pushed to git repo; I updated commit sha1. New commits:
405ef3e  Merge branch 'u/ghkliem/first_parallel_version_of_face_iterator_reb' of git://trac.sagemath.org/sage into u/ghkliem/first_parallel_version_of_face_iterator_reb2

comment:24 in reply to: ↑ 19 Changed 5 weeks ago by
This bug isn't too hard to reproduce actually:
https://github.com/kliem/sage/runs/2064520070?check_suite_focus=true
Seems to be connected to older GCC.
Replying to vbraun:
Build fails on various buildbots:
[sagelib9.3.beta7] [1/1] gcc Wnounusedresult Wsigncompare DNDEBUG g fwrapv O3 Wall Wnounused O2 g march=native O2 g march=native fPIC UCYSIGNALS_C_ATOMIC I/home/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/cysignals I./sage/data_structures I./sage/cpython Isage/cpython I/home/buildbot/slave/sage_git/build/build/pkgs/sagelib/src I/home/buildbot/slave/sage_git/build/local/include/python3.9 I/home/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/numpy/core/include Ibuild/cythonized I/home/buildbot/slave/sage_git/build/local/include/python3.9 c build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c o build/temp.linuxx86_643.9/build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.o fnostrictaliasing DCYTHON_CLINE_IN_TRACEBACK=1 fopenmp std=c99 [sagelib9.3.beta7] In file included from build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:662:0: [sagelib9.3.beta7] /home/buildbot/slave/sage_git/build/local/lib/python3.9/sitepackages/cysignals/struct_signals.h:45:1: sorry, unimplemented: ‘_Atomic’ with OpenMP [sagelib9.3.beta7] typedef volatile _Atomic int cy_atomic_int; [sagelib9.3.beta7] ^~~~~~~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c: In function ‘__pyx_f_4sage_8geometry_10polyhedron_24combinatorial_polyhedron_13face_iterator_prepare_face_iterator_for_partial_job’: [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9037:54: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta7] __pyx_t_2 = ((__pyx_v_structure>current_dimension == (__pyx_v_structure>dimension  __pyx_v_parallelization_depth)) != 0); [sagelib9.3.beta7] ^~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9324:84: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta7] __pyx_t_1 = (((__pyx_v_parallel_struct>current_job_id[__pyx_v_current_depth]) == 1L) != 0); [sagelib9.3.beta7] ^~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:9675:54: warning: comparison between signed and unsigned integer expressions [Wsigncompare] [sagelib9.3.beta7] __pyx_t_1 = ((__pyx_v_structure>current_dimension != ((__pyx_v_structure>dimension  __pyx_v_parallelization_depth)  1)) != 0); [sagelib9.3.beta7] ^~ [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c: In function ‘__Pyx_InitGlobals’: [sagelib9.3.beta7] build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:23285:1: warning: ‘PyEval_InitThreads’ is deprecated [Wdeprecateddeclarations] [sagelib9.3.beta7] PyEval_InitThreads(); [sagelib9.3.beta7] ^~~~~~~~~~~~~~~~~~ [sagelib9.3.beta7] In file included from /home/buildbot/slave/sage_git/build/local/include/python3.9/Python.h:145:0, [sagelib9.3.beta7] from build/cythonized/sage/geometry/polyhedron/combinatorial_polyhedron/face_iterator.c:54: [sagelib9.3.beta7] /home/buildbot/slave/sage_git/build/local/include/python3.9/ceval.h:130:37: note: declared here [sagelib9.3.beta7] Py_DEPRECATED(3.9) PyAPI_FUNC(void) PyEval_InitThreads(void); [sagelib9.3.beta7] ^~~~~~~~~~~~~~~~~~ [sagelib9.3.beta7] error: command '/usr/bin/gcc' failed with exit code 1 [sagelib9.3.beta7] [sagelib9.3.beta7] real 0m3.325s [sagelib9.3.beta7] user 0m2.798s [sagelib9.3.beta7] sys 0m0.563s Makefile:2233: recipe for target 'sagelibnodeps' failed
comment:25 Changed 5 weeks ago by
 Dependencies changed from #31226, #31455 to #31474
comment:26 Changed 4 weeks ago by
 Status changed from needs_review to needs_work
This might need more work, as the flag fopenmp
isn't understood everywhere: https://github.com/kliem/sage/pull/40/checks?check_run_id=2076988665
So we should probably check out at configure time, whether our compiler supports this, similar to https://trac.sagemath.org/ticket/27122.
comment:27 Changed 4 weeks ago by
 Dependencies changed from #31474 to #31474, #31499
comment:28 Changed 4 weeks ago by
 Commit changed from 405ef3e3bb65ac1c610727c590daf75c19048c45 to 27baa071a64d5f93ca49d5ca4c18ae8ca7fa892a
Branch pushed to git repo; I updated commit sha1. New commits:
55a4634  merge with current develop

6edd48b  define cython aliases OPENMP_CFLAGS and OPENMP_CXXFLAGS

71f8950  fixes to get the aliases to work

b76dc3c  add doctest that shows how use cython.parallel.prange

07cb470  Merge branch 'u/ghkliem/check_openmp_at_configuration' of git://trac.sagemath.org/sage into u/ghkliem/first_parallel_version_of_face_iterator_reb

27baa07  use OPENMP_CFLAGS

comment:29 Changed 4 weeks ago by
 Status changed from needs_work to needs_review
comment:30 Changed 4 weeks ago by
Tests are running here:
comment:31 Changed 4 weeks ago by
 Commit changed from 27baa071a64d5f93ca49d5ca4c18ae8ca7fa892a to ae482a4d07ebc5f68fe78b37a32919c85b94aef5
The
sig_check()
outside the loop is unnecessary:Likewise here:
Here you either need the
sig_on/sig_off
or some more targeted checking. I don't see the point in performing a single check here.Otherwise LGTM.