Enh/ml beneath beyond #7

kalmarek · 2020-09-16T09:31:29Z

I kept beneath_beyond_algo_for_ml in polymake::polytope namespace, for it was much easier to overload methods without prefixing, but that can be changed if not desired.

…t an alias also properly fill the NodeMap and EdgeMap

kalmarek · 2020-11-05T13:13:29Z

I rebased onto master;

there are some silly names (class beneath_beyond_algo_for_ml: public beneath_beyond_algo)
the class is injected into polymake::polytope namespace;

If it were possible (with little effort) I'd keep it as a separate library (it's of very specialized use), but all solutions I could think of are rather elaborate, hence this pull.

@benlorenz let me know what do you think.

benlorenz · 2020-11-05T13:34:29Z

Please also update the corresponding branch in Polymake.jl for the tests to succeed.

Apart from that it is fine I think, I will have a closer look later. For the name we could maybe use beneath_beyond_wrapper instead. The main difference is that it is copyable and can be used step-wise, is this correct?

kalmarek · 2020-11-05T14:07:11Z

Apart from that it is fine I think, I will have a closer look later. For the name we could maybe use beneath_beyond_wrapper instead. The main difference is that it is copyable and can be used step-wise, is this correct?

yes; maybe then beneath_beyond_step_by_step :D (on a more serious note: beneath_beyond_stepwise sounds ok, and is more descriptive than wrapper.

Please also update the corresponding branch in Polymake.jl for the tests to succeed.

will do; will it automatically detect enh/ml_beneath_beyond branch?

benlorenz · 2020-11-06T08:47:59Z

Apart from that it is fine I think, I will have a closer look later. For the name we could maybe use beneath_beyond_wrapper instead. The main difference is that it is copyable and can be used step-wise, is this correct?

yes; maybe then beneath_beyond_step_by_step :D (on a more serious note: beneath_beyond_stepwise sounds ok, and is more descriptive than wrapper.

That's also fine yes, I just wanted something other than _for_ml which doen't really fit.

Please also update the corresponding branch in Polymake.jl for the tests to succeed.

will do; will it automatically detect enh/ml_beneath_beyond branch?

Yes, it will look for a branch in Polymake.jl with the same name as the branch here and use that (or fall back to master), see https://github.com/oscar-system/libpolymake-julia/pull/7/checks?check_run_id=1358349236#step:11:15.

kalmarek · 2020-11-09T11:09:05Z

ok, so now I get crashes in clear:246 (after adding a few points and going out of scope):

signal (11): Segmentation fault
in expression starting at REPL[12]:1
operator[] at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/SeriesRaw.h:229 [inlined]
elem_by_index at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/modified_containers.h:1217
operator[] at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/modified_containers.h:1264 [inlined]
row at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/matrix_methods.h:199 [inlined]
clear at /home/kalmar/local/src/libpolymake-julia/src/beneath_beyond.cpp:246
operator() at /usr/include/c++/10.2.0/bits/std_function.h:622 [inlined]
operator() at /home/kalmar/.julia/artifacts/fd5dfb5dee87c41c238d98bd7ff2fdd4f307e824/include/jlcxx/module.hpp:58 [inlined]
apply at /home/kalmar/.julia/artifacts/fd5dfb5dee87c41c238d98bd7ff2fdd4f307e824/include/jlcxx/module.hpp:72
_bb_clear! at /home/kalmar/.julia/packages/CxxWrap/ZOkSN/src/CxxWrap.jl:590 [inlined]
_bb_clear! at /home/kalmar/.julia/dev/Polymake/src/beneath_beyond.jl:74
_jl_invoke at /buildworker/worker/package_linux64/build/src/gf.c:2231 [inlined]
jl_apply_generic at /buildworker/worker/package_linux64/build/src/gf.c:2398
jl_apply at /buildworker/worker/package_linux64/build/src/julia.h:1690 [inlined]
run_finalizer at /buildworker/worker/package_linux64/build/src/gc.c:277
jl_gc_run_finalizers_in_list at /buildworker/worker/package_linux64/build/src/gc.c:363
run_finalizers at /buildworker/worker/package_linux64/build/src/gc.c:391 [inlined]
jl_gc_collect at /buildworker/worker/package_linux64/build/src/gc.c:3127
gc at ./gcutils.jl:79 [inlined]
gc at ./gcutils.jl:79

or

signal (11): Segmentation fault
in expression starting at REPL[6]:1
set_data<const pm::Rational&> at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/next/Rational.h:1836 [inlined]
Rational at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/next/Rational.h:213 [inlined]
construct_at<pm::Rational, const pm::Rational&> at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/type_manip.h:1474 [inlined]
init_from_sequence<pm::ptr_wrapper<const pm::Rational, false> > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/shared_object.h:717 [inlined]
init_from_iterator<pm::ptr_wrapper<const pm::Rational, false>, pm::shared_array<pm::Rational, pm::AliasHandlerTag<pm::shared_alias_handler> >::rep::copy> at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/shared_object.h:740 [inlined]
init<pm::shared_array<pm::Rational, pm::AliasHandlerTag<pm::shared_alias_handler> >::rep::copy, pm::ptr_wrapper<const pm::Rational, false> > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/shared_object.h:858 [inlined]
init<pm::ptr_wrapper<const pm::Rational, false> > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/shared_object.h:869 [inlined]
construct_copy<pm::ptr_wrapper<const pm::Rational, false> > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/shared_object.h:909 [inlined]
assign<pm::ptr_wrapper<const pm::Rational, false> > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/shared_object.h:1194
assign<pm::IndexedSlice<pm::masquerade<pm::ConcatRows, const pm::Matrix_base<pm::Rational>&>, const pm::Series<long int, true>, polymake::mlist<> > > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/next/Vector.h:163 [inlined]
operator=<pm::IndexedSlice<pm::masquerade<pm::ConcatRows, const pm::Matrix_base<pm::Rational>&>, const pm::Series<long int, true>, polymake::mlist<> >, pm::Rational> at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/GenericVector.h:242 [inlined]
clear at /home/kalmar/local/src/libpolymake-julia/src/beneath_beyond.cpp:246
operator() at /usr/include/c++/10.2.0/bits/std_function.h:622 [inlined]
operator() at /home/kalmar/.julia/artifacts/fd5dfb5dee87c41c238d98bd7ff2fdd4f307e824/include/jlcxx/module.hpp:58 [inlined]
apply at /home/kalmar/.julia/artifacts/fd5dfb5dee87c41c238d98bd7ff2fdd4f307e824/include/jlcxx/module.hpp:72
_bb_clear! at /home/kalmar/.julia/packages/CxxWrap/ZOkSN/src/CxxWrap.jl:590 [inlined]
_bb_clear! at /home/kalmar/.julia/dev/Polymake/src/beneath_beyond.jl:74
_jl_invoke at /buildworker/worker/package_linux64/build/src/gf.c:2214 [inlined]
jl_apply_generic at /buildworker/worker/package_linux64/build/src/gf.c:2398
jl_apply at /buildworker/worker/package_linux64/build/src/julia.h:1690 [inlined]
run_finalizer at /buildworker/worker/package_linux64/build/src/gc.c:277
jl_gc_run_finalizers_in_list at /buildworker/worker/package_linux64/build/src/gc.c:363
run_finalizers at /buildworker/worker/package_linux64/build/src/gc.c:391 [inlined]
jl_gc_collect at /buildworker/worker/package_linux64/build/src/gc.c:3127
gc at ./gcutils.jl:79 [inlined]
gc at ./gcutils.jl:79

benlorenz · 2020-11-09T11:32:08Z

I am slightly confused why clear is run in the finalizer when the object is being destroyed anyway, it doesn't seem to be needed for cleaning up but more for making sure to have a nice state before retrieving results.
The backtrace looks like either the points pointer is not valid anymore or vertices_so_far is empty so front() returns something invalid.
Can you post a code snipped to reproduce this?

kalmarek · 2020-11-09T11:46:18Z

oh, I see; I thought clear needs to be run upon exit to cleanup the memory (possibly) owned by beneath_beyond_algo

here's a snippet to stress-out deepcopy:

using Polymake,Random

const N = 5
min_perm = collect(1:2^N)

min_size = let c = polytope.cube(N)
    bb = Polymake.BeneathBeyond(deepcopy(c.VERTICES), perm=min_perm)
    collect(bb)
    Polymake.triangulation_size(bb)
end


while true
    c = polytope.cube(N)
    bb = GC.@preserve c begin
        Polymake.BeneathBeyond(deepcopy(c.VERTICES))
    end

    perm = randperm(2^N)

    for n in perm
        bb = deepcopy(bb)
        Polymake.add_point!(bb, n)
    end

    s = Polymake.triangulation_size(bb)
    if s < min_size
        min_perm = perm
        min_size = s
        println("")
        @info Polymake.triangulation_size(bb)
    else
        print(".")
    end
end

this run endlessly without bb = deepcopy(bb) line; with it you get strange behavior, like

ERROR: Integer/Rational NaN
Stacktrace:^[[A
 [1] _bb_add_point! at /home/kalmar/.julia/packages/CxxWrap/ZOkSN/src/CxxWrap.jl:590 [inlined]
 [2] add_point!(::Polymake.BeneathBeyond{Polymake.Rational}, ::Int64) at /home/kalmar/.julia/dev/Polymake/src/beneath_beyond.jl:48
 [3] top-level scope at ./REPL[5]:11

or

signal (11): Segmentation fault
in expression starting at REPL[5]:1
operator* at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/next/Rational.h:959
operator() at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/operations_basic_defs.h:123 [inlined]
operator* at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/iterators.h:2565 [inlined]
accumulate<pm::TransformedContainerPair<pm::Vector<pm::Rational>&, pm::IndexedSlice<pm::masquerade<pm::ConcatRows, const pm::Matrix_base<pm::Rational>&>, const pm::Series<long int, true>, polymake::mlist<> >&, pm::BuildBinary<pm::operations::mul> >, pm::BuildBinary<pm::operations::add> > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/internal/operations.h:1114 [inlined]
operator*<pm::Vector<pm::Rational>&, pm::IndexedSlice<pm::masquerade<pm::ConcatRows, const pm::Matrix_base<pm::Rational>&>, const pm::Series<long int, true>, polymake::mlist<> > > at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/GenericVector.h:531
coord_full_dim at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/polytope/beneath_beyond_impl.h:985
update_facets at /home/kalmar/.julia/artifacts/f115f08730885160f615c5575bccf08aa26eba76/include/polymake/polytope/beneath_beyond_impl.h:835
process_point at /home/kalmar/local/src/libpolymake-julia/src/beneath_beyond.cpp:183
operator() at /usr/include/c++/10.2.0/bits/std_function.h:622 [inlined]
operator() at /home/kalmar/.julia/artifacts/fd5dfb5dee87c41c238d98bd7ff2fdd4f307e824/include/jlcxx/module.hpp:58 [inlined]
apply at /home/kalmar/.julia/artifacts/fd5dfb5dee87c41c238d98bd7ff2fdd4f307e824/include/jlcxx
_bb_add_point! at /home/kalmar/.julia/packages/CxxWrap/ZOkSN/src/CxxWrap.jl:590 [inlined]
add_point! at /home/kalmar/.julia/dev/Polymake/src/beneath_beyond.jl:48
unknown function (ip: 0x7f665ef87109)

the snippets crashing with GC.gc() were more complicated, I'm still trying to reproduce them in isolation

benlorenz · 2020-11-09T12:49:31Z

Probably clear should be renamed then.

deepcopy for BeneathBeyond in beneath_beyond.jl should not deepcopy rays and lineality. These are stored as pointers inside bb.algo, so we need references to the original matrices to keep those alive.

-            copy(bb.rays),
-            copy(bb.lineality),
+            bb.rays,
+            bb.lineality,

Or we need something like adjust_ptrs(bb.algo,bb.rays,bb.lineality) that changes the pointers within the algo object to the new copies (depending on the state just source_points or points as well, and the same for linealities).

With this change it is running for a few minutes now (with the deepcopy in place).

kalmarek · 2020-11-09T13:28:45Z

thanks! that's spot on and does the trick!

After reading through code of clear, the name doesn't make much sense indeed;
any suggestion? maybe mopup?

benlorenz · 2020-11-09T13:54:14Z

finish, finalize (is probably bad in the julia context?), postprocess ?
There is not much cleaning involved, it will make sure the facet normals are valid, add the empty facet and the missing simplex when the polytope is just a point and make sure that the graph has a sequential node numbers in the full-dimensional case (no gaps).

fieker · 2020-11-09T14:40:17Z

On Mon, Nov 09, 2020 at 05:54:29AM -0800, Benjamin Lorenz wrote: `finish`, `finalize` (is probably bad in the julia context?), `postprocess` ? There is not much cleaning involved, it will make sure the facet normals are valid, add the empty facet and the missing simplex when the polytope is just a point and make sure that the graph has a sequential node numbers in the full-dimensional case (no gaps).

finish! is what we use to finialze assembly of multivariate polynomials (MPolyBuildCtx)...

…

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #7 (comment)

kalmarek · 2020-11-12T10:14:40Z

while true
    let N = 5, c = polytope.cube(N)
        @assert N >= 3
        pts = c.VERTICES
        bb = Polymake.BeneathBeyond(pts)
        sizes = Vector{Int}(undef, 100)
        for p in rand(1:2^(N-2))
            Polymake.add_point!(bb, p)
        end
        Threads.@threads for i = 1:length(sizes)
            bb_new = deepcopy(bb)
            bb_new.perm = randperm(2^N)
            collect(bb_new) # will only add new points
            sizes[i] = Polymake.triangulation_size(bb_new)
        end
        @info "minimal size $(minimum(sizes))"
    end
end

on the corresponding Polymake.jl branch this crashes with rather short message, e.g.

signal (11): Segmentation fault
in expression starting at REPL[1]:1
malloc(): unsorted double linked list corrupted

(this runs just fine is you set JULIA_NUM_THREADS=1)

benlorenz · 2020-11-24T14:29:53Z

Changes for the script:

deepcopy of the BeneathBeyond objects needs to happen in the main thread so we prepare those before launching the threads.
I wanted to test different numbers of initial points so I added an extra dimension to the rand().

while true
           let N = 5, c = polytope.cube(N)
               @assert N >= 3
               pts = c.VERTICES
               bb = Polymake.BeneathBeyond(pts)
               sizes = Vector{Int}(undef, 100)
               for p in rand(1:2^N,rand(1:2^(N-1)))
                   Polymake.add_point!(bb, p)
               end
               bbs = Vector{Polymake.BeneathBeyond}(undef,length(sizes))
               for i in 1:length(bbs)
                   bbs[i] = deepcopy(bb)
               end
               Threads.@threads for i = 1:length(sizes)
                   bbs[i].perm = randperm(2^N)
                   collect(bbs[i]) # will only add new points
                   sizes[i] = Polymake.triangulation_size(bbs[i])
               end
               @info "minimal size $(minimum(sizes))"
           end
       end

Adjusted binaries for polymake_jll and libpolymake_julia_jll are currently building and will be available here (if all the binarybuilder stuff works correctly):

polymake_jll with patch at https://github.com/benlorenz/polymake_jll.jl
libpolymake_julia_jll from this branch using the patched polymake_jll at https://github.com/benlorenz/libpolymake_julia_jll.jl

When the binaries are uploaded you can try dev'ing those two repos. Then the above script might work with JULIA_NUM_THREADS > 1.

benlorenz · 2020-11-24T23:08:57Z

I pushed some extra locking to the Polymake.jl branch that should allow copying from inside the thread as well.
(I first tried to implement the locking in C++ but that resulted in deadlocks with the julia GC ...)

while true 
           let N = 5, c = polytope.cube(N)
               @assert N >= 3
               pts = c.VERTICES
               bb = Polymake.BeneathBeyond(pts)
               sizes = Vector{Int}(undef, 100)
               for p in rand(1:2^N,rand(1:2^(N-1)))
                   Polymake.add_point!(bb, p)
               end
               Threads.@threads for i = 1:length(sizes)
                   bbcopy = deepcopy(bb)
                   bbcopy.perm = randperm(2^N)
                   collect(bbcopy) # will only add new points
                   sizes[i] = Polymake.triangulation_size(bbcopy)
               end
               @info "minimal size $(minimum(sizes))"
           end
       end

kalmarek requested a review from benlorenz September 16, 2020 09:31

kalmarek and others added 7 commits November 5, 2020 14:03

initial version of BB

ffedd22

add copy constructor

fe9e012

move add_beneath_beyond to jlpolymake namespace

5e4c2d6

tidy names: prefix bb_* methods with "_"

244d89f

first try on the complete copy constructors

abe2307

ml_beneath_beyond: copy graph via copy_permuted to avoid creating jus…

74bfc30

…t an alias also properly fill the NodeMap and EdgeMap

ml_beneath_beyond: try making clang on macos happy

b943655

kalmarek force-pushed the enh/ml_beneath_beyond branch from 51e4934 to b943655 Compare November 5, 2020 13:06

rename beneath_beyond_algo_for_ml → beneath_beyond_algo_stepwise

1ddbb4e

kalmarek added 2 commits November 11, 2020 15:10

rename clear → finish

d3625eb

provisional deepcopy of beneath_beyond_algo_stepwise

f5fbb1c

kalmarek and others added 4 commits November 16, 2020 11:24

deepcopy bb.AH and bb.facet_nullspace

90ea63e

explicitly copy Bitsets and deque used in per-step operation

3e9d898

more copies, store full points as well

5b5dd44

Merge remote-tracking branch 'origin/master' into enh/ml_beneath_beyond

7d0bad3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enh/ml beneath beyond #7

Enh/ml beneath beyond #7

kalmarek commented Sep 16, 2020

kalmarek commented Nov 5, 2020

benlorenz commented Nov 5, 2020

kalmarek commented Nov 5, 2020

benlorenz commented Nov 6, 2020

kalmarek commented Nov 9, 2020

benlorenz commented Nov 9, 2020

kalmarek commented Nov 9, 2020

benlorenz commented Nov 9, 2020

kalmarek commented Nov 9, 2020

benlorenz commented Nov 9, 2020

fieker commented Nov 9, 2020 via email

kalmarek commented Nov 12, 2020

benlorenz commented Nov 24, 2020

benlorenz commented Nov 24, 2020 •

edited

Loading

Enh/ml beneath beyond #7

Are you sure you want to change the base?

Enh/ml beneath beyond #7

Conversation

kalmarek commented Sep 16, 2020

kalmarek commented Nov 5, 2020

benlorenz commented Nov 5, 2020

kalmarek commented Nov 5, 2020

benlorenz commented Nov 6, 2020

kalmarek commented Nov 9, 2020

benlorenz commented Nov 9, 2020

kalmarek commented Nov 9, 2020

benlorenz commented Nov 9, 2020

kalmarek commented Nov 9, 2020

benlorenz commented Nov 9, 2020

fieker commented Nov 9, 2020 via email

kalmarek commented Nov 12, 2020

benlorenz commented Nov 24, 2020

benlorenz commented Nov 24, 2020 • edited Loading

benlorenz commented Nov 24, 2020 •

edited

Loading