CxxWrap.jl icon indicating copy to clipboard operation
CxxWrap.jl copied to clipboard

Getting an Array{Int64,1} from C++ function - bad values.

Open TransGirlCodes opened this issue 5 years ago • 5 comments

Hey @barche

I have a class wrapped:

mod.add_type<SequenceGraph>("SequenceGraph")
            .constructor()
            .method("load_from_gfa", &SequenceGraph::load_from_gfa)

The class has the following method that returns a std::vector:

    std::vector<sgNodeID_t> get_fw_nodes(sgNodeID_t n) const;

I want to wrap this in CxxWrap so I did so using a lambda, that creates an jlcxx::ArrayRef, from the vector, and returns that:

.method("get_fw_nodes", [](SequenceGraph& sg, sgNodeID_t n) {
                std::vector<sgNodeID_t> fw_nodes = sg.get_fw_nodes(n);
                for(const auto i:fw_nodes) {std::cout << i << std::endl;}
                jlcxx::ArrayRef<sgNodeID_t> jlarr(true, fw_nodes.data(), fw_nodes.size());
                for(const auto j:jlarr) {std::cout << j << std::endl;}
                return jlarr;
            })

The for loops are just to print the vector, and ArrayRef contents to make sure they are the same.

However, when I then run this from Julia, the returned array has values that are not the same as those on the C++ side:

julia> using BSG
[ Info: Precompiling BSG [3c37b7a2-d3b5-11e8-3e9e-e917918d9941]
WARNING: Method definition (::Type{BSG.KmerIDX})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.KmerIDX128})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.graphStrandPos})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.nodeVisitor})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.Link})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.SequenceGraph})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.WorkSpace})() in module BSG overwritten.

julia> ws = BSG.WorkSpace() # Not important, just loading a graph from disk...
BSG.WorkSpaceAllocated(Ptr{Nothing} @0x00007ffdb162f430)

julia> BSG.load_from_disk(ws, "/Users/bward/Desktop/ecoli_workspace/workspace_kci_ecoli.bsgws", false)

2018-11-15 13:15:00: Loaded graph with 210 nodes
2018-11-15 13:15:00: KCI Mean coverage for unique kmers:   84.2364
2018-11-15 13:15:00: KCI Median coverage for unique kmers: 84
2018-11-15 13:15:00: KCI Mode coverage for unique kmers:   80

julia> sg = BSG.getGraph(ws)
BSG.SequenceGraphRef(Ptr{Nothing} @0x00007ffdb162f448)

julia> BSG.get_fw_nodes(sg, 5) # Ok now I want to get the array from the C++ side.
48
58
48
58
2-element Array{Int64,1}:
      4620726880
 140727552842608

TransGirlCodes avatar Nov 15 '18 13:11 TransGirlCodes

@barche

I suspected the issue was that the ArrayRef I was returning, used the data pointer from a vector, and perhaps the ArrayRef did not copy data from the vector, but simply used the very same pointer. So whilst ArrayRef got passed to julia, when vector went out of scope, it frees the memory. So I got around that with this:

.method("get_fw_nodes", [](SequenceGraph& sg, sgNodeID_t n) {
                std::vector<sgNodeID_t> fw_nodes = sg.get_fw_nodes(n);
                sgNodeID_t *nodes = new sgNodeID_t[fw_nodes.size()];
                std::copy(fw_nodes.begin(), fw_nodes.end(), nodes);
                auto jlarr = jlcxx::make_julia_array(nodes, fw_nodes.size());
                for(const auto j:jlarr) {std::cout << j << std::endl;}
                return jlarr;
            })

Now whilst the use of new without a paired delete scares the bejeebus out of me, I'm trusting julia gc to free the memory (I can see make_julia_array uses an ArrayRef constructor with the boolean option for julia owning the memory set to true). It seems to work:

julia> using BSG
[ Info: Recompiling stale cache file /Users/bward/.julia/compiled/v1.0/BSG/abDuO.ji for BSG [3c37b7a2-d3b5-11e8-3e9e-e917918d9941]
WARNING: Method definition (::Type{BSG.KmerIDX})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.KmerIDX128})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.graphStrandPos})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.nodeVisitor})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.Link})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.SequenceGraph})() in module BSG overwritten.
WARNING: Method definition (::Type{BSG.WorkSpace})() in module BSG overwritten.

julia> ws = BSG.WorkSpace()
BSG.WorkSpaceAllocated(Ptr{Nothing} @0x00007fc1bd431850)

julia> 

julia> BSG.load_from_disk(ws, "/Users/bward/Desktop/ecoli_workspace/workspace_kci_ecoli.bsgws", false)
2018-11-15 21:25:36: Loaded graph with 210 nodes
2018-11-15 21:25:36: KCI Mean coverage for unique kmers:   84.2364
2018-11-15 21:25:36: KCI Median coverage for unique kmers: 84
2018-11-15 21:25:36: KCI Mode coverage for unique kmers:   80

julia> sg = BSG.getGraph(ws)
BSG.SequenceGraphRef(Ptr{Nothing} @0x00007fc1bd431868)

julia> fwn = BSG.get_fw_nodes(sg, 5)
48
58
2-element Array{Int64,1}:
 48
 58

TransGirlCodes avatar Nov 15 '18 22:11 TransGirlCodes

I'm not sure if this really will work in all cases, allocating memory on the C++ side and freeing it on the Julia side could cause problems. You should also be able to use jlcxx::Array in this case I think:

 .method("get_fw_nodes", [](SequenceGraph& sg, sgNodeID_t n) {
                std::vector<sgNodeID_t> fw_nodes = sg.get_fw_nodes(n);
                jlcxx::Array<sgNodeID_t> nodes;
                for(sgNodeID_t nid : fw_nodes) {
                    nodes.push_back(nid);
                }
                return nodes;
            })

barche avatar Nov 19 '18 20:11 barche

I tried a different tack, and tried to wrap std::vector:

mod.add_type<jlcxx::Parametric<jlcxx::TypeVar<1>>>("BSGVector")
        .apply<std::vector<int>, std::vector<char>>([](auto wrapped){
            typedef typename decltype(wrapped)::type WrappedT;
            wrapped.method("size", &WrappedT::size);
            //wrapped.method("at", [](WrappedT& vec, size_t i) { return vec.at(i); });
        });

As a lightweight version, this compiles fine in C++, but on the julia side I get the nullptr error, which previoudly meant that a type wasn't wrapped... yet in the above example I eliminated all my classes, which are wrapped anyway, and just put in int and char and it still resulted in:

julia> using BSG
[ Info: Precompiling BSG [3c37b7a2-d3b5-11e8-3e9e-e917918d9941]
Assertion failed: (type_pointer() != nullptr), function julia_type, file /Users/bward/Desktop/libcxxwrap-julia/include/jlcxx/type_conversion.hpp, line 319.

signal (6): Abort trap: 6
in expression starting at /Users/bward/Desktop/BSG/src/BSG.jl:5
__pthread_kill at /usr/lib/system/libsystem_kernel.dylib (unknown line)
Allocations: 2914204 (Pool: 2913574; Big: 630); GC: 5
ERROR: Failed to precompile BSG [3c37b7a2-d3b5-11e8-3e9e-e917918d9941] to /Users/bward/.julia/compiled/v1.0/BSG/abDuO.ji.
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] macro expansion at ./logging.jl:313 [inlined]
 [3] compilecache(::Base.PkgId, ::String) at ./loading.jl:1187
 [4] _require(::Base.PkgId) at ./logging.jl:311
 [5] require(::Base.PkgId) at ./loading.jl:855
 [6] macro expansion at ./logging.jl:311 [inlined]
 [7] require(::Module, ::Symbol) at ./loading.jl:837

Getting rid of the above wrapping code makes the error go away.

It would be nice if CxxWrap might be able to be more specific about which type or method is triggering this error in future releases, as I only found it was the above vector lines after repeatedly commenting out stuff.

TransGirlCodes avatar Nov 20 '18 01:11 TransGirlCodes

@benjward you may be interested in checking Xtensor.jl / xtensor. The julia bindings are built upon cxxwrap and the c++ API is very similar to that of numpy. You should be able to return a jlarray or jltensor from you C++ code.

SylvainCorlay avatar Nov 20 '18 07:11 SylvainCorlay

Ah, actually I had put in that assert for debugging purposes and forgot to remove it. If you build from libcxxwrap master or just remove the assert yourself, an exception with the mangled name of the type should be thrown instead. You can then unmangle this name with the shell command:

c++filt -t mangled_type_name

barche avatar Nov 20 '18 17:11 barche