marray API changes?

[bluescarni]

It seems like nifty at this time is including an older version of marray (with the latest upstream commit dating to 2014). The marray which is packaged in conda however is the release 3.1 from 2016:

https://github.com/conda-forge/marray-feedstock

The two marray versions are API incompatible, so it would be good if possible if nifty could be updated to be compatible with the latest marray API.

(currently working to use external dependencies, with eye towards #55)

[constantinpape]

Do you know which parts of the API are incompatible?
I could try to build with the newer version later and see if I can fix build issues.

[bluescarni]

This is the first error message I am getting:

[  3%] Building CXX object src/python/lib/graph/CMakeFiles/_graph.dir/undirected_grid_graph.cxx.o
In file included from /home/bluescarni/repos/nifty/include/nifty/marray/marray.hxx:8:0,
                 from /home/bluescarni/repos/nifty/include/nifty/graph/graph_maps.hxx:3,
                 from /home/bluescarni/repos/nifty/include/nifty/graph/undirected_graph_base.hxx:8,
                 from /home/bluescarni/repos/nifty/include/nifty/graph/undirected_grid_graph.hxx:11,
                 from /home/bluescarni/repos/nifty/src/python/lib/graph/undirected_grid_graph.cxx:10:
/home/bluescarni/miniconda3/envs/nifty/include/andres/marray.hxx: In instantiation of ‘static typename andres::View<T, isConst, A>::reference andres::marray_detail::AccessOperatorHelper<false>::execute(const andres::View<T, isConst, A>&, U) [with T = float; U = std::array<long int, 2ul>; bool isConst = false; A = std::allocator<long unsigned int>; typename andres::View<T, isConst, A>::reference = float&]’:
/home/bluescarni/miniconda3/envs/nifty/include/andres/marray.hxx:1148:92:   required from ‘andres::View<T, isConst, A>::reference andres::View<T, isConst, A>::operator()(U) const [with U = std::array<long int, 2ul>; T = float; bool isConst = false; A = std::allocator<long unsigned int>; andres::View<T, isConst, A>::reference = float&]’
/home/bluescarni/repos/nifty/include/nifty/graph/undirected_grid_graph.hxx:300:36:   required from ‘void nifty::graph::UndirectedGridGraph<DIM, true>::imageToEdgeMap(const IMAGE&, BINARY_FUNCTOR, EDGE_MAP&) const [with IMAGE = nifty::marray::PyView<float, 2ul, true>; BINARY_FUNCTOR = nifty::graph::exportUndirectedGridGraphT(pybind11::module&)::<lambda(const GraphType&, nifty::marray::PyView<float, DIM>, const string&)> [with long unsigned int DIM = 2ul; GraphType = nifty::graph::UndirectedGridGraph<2ul, true>; std::__cxx11::string = std::__cxx11::basic_string<char>]::<anonymous struct>; EDGE_MAP = nifty::marray::PyView<float>; long unsigned int DIM = 2ul]’
/home/bluescarni/repos/nifty/src/python/lib/graph/undirected_grid_graph.cxx:64:25:   required from ‘nifty::graph::exportUndirectedGridGraphT(pybind11::module&)::<lambda(const GraphType&, nifty::marray::PyView<float, DIM>, const string&)> [with long unsigned int DIM = 2ul; GraphType = nifty::graph::UndirectedGridGraph<2ul, true>; std::__cxx11::string = std::__cxx11::basic_string<char>]’
/home/bluescarni/repos/nifty/src/python/lib/graph/undirected_grid_graph.cxx:50:18:   required from ‘struct nifty::graph::exportUndirectedGridGraphT(pybind11::module&) [with long unsigned int DIM = 2ul]::<lambda(const GraphType&, class nifty::marray::PyView<float, 2ul, true>, const string&)>’
/home/bluescarni/repos/nifty/src/python/lib/graph/undirected_grid_graph.cxx:32:9:   required from ‘void nifty::graph::exportUndirectedGridGraphT(pybind11::module&) [with long unsigned int DIM = 2ul]’
/home/bluescarni/repos/nifty/src/python/lib/graph/undirected_grid_graph.cxx:162:45:   required from here
/home/bluescarni/miniconda3/envs/nifty/include/andres/marray.hxx:5665:57: error: no match for ‘operator*’ (operand type is ‘std::array<long int, 2ul>’)
         Assert(MARRAY_NO_DEBUG || v.dimension() != 0 || *it == 0);
                                                         ^~~
/home/bluescarni/miniconda3/envs/nifty/include/andres/marray.hxx:3131:1: note: candidate: template<class E1, class T1, class E2, class T2> const andres::BinaryViewExpression<E1, T1, E2, T2, andres::marray_detail::Times<T1, T2, typename andres::marray_detail::PromoteType<T1, T2>::type> > andres::operator*(const andres::ViewExpression<E1, T1>&, const andres::ViewExpression<E2, T2>&)
 operator*
 ^~~~~~~~
/home/bluescarni/miniconda3/envs/nifty/include/andres/marray.hxx:3131:1: note:   template argument deduction/substitution failed:
/home/bluescarni/miniconda3/envs/nifty/include/andres/marray.hxx:5665:57: note:   ‘std::array<long int, 2ul>’ is not derived from ‘const andres::ViewExpression<E1, T1>’
         Assert(MARRAY_NO_DEBUG || v.dimension() != 0 || *it == 0);

I suppose it's related to this commit?

DerThorsten/marray@8df23c1

[bluescarni]

Maybe the additional std::array overloads could be moved into nifty code for the time being.

[constantinpape]

Ah ok, I see the issue is that we are using Thorstens custom version of marray...

Yes you are right, it's probably the best idea to move the additional stuff to nifty.

[DerThorsten]

Lets just derive from marray in nifty and use this class.
In fact i already patch the namespaces such that it looks like marray::Marray is in nifty::marray::Marray

[DerThorsten]

Problem:
We use andres::View and andres::Marray which is derived from andres::View.
If make MyView derive from andres::View and MyMarray derive from andres::Marray we have a problem.
MyMarray is NOT derived from MyView.
But our codebase is sometimes reliant on that.

Any ideas?

[bluescarni]

Is there any way of patching the class from the outside rather than doing an intrusive modification via member functions?

[DerThorsten]

not really.
My Favorite Options atm:

• cleanup the changes in maray::View and make a pr
• we just copy marray into our include path and hack/patch whatever we want (I know i general this is a very bad idea, but for marray I would be very fine with this)

[constantinpape]

What if we inherit with nifty::marray::View from andres::View and with nifty::marray from nifty::marray::View and andres::Marray ?
Dunno if this is a good idea in terms of figuring out who owns the memory propblem etc.
But it should sovlve the 'Use nifty::marray::Marray as nifty::marray::View' problem.

[bluescarni]

I'd favor the PR option in the long run, but it also depends on the likelihood/timeliness of it getting merged...

[DerThorsten]

Ok, then for the moment we can copy marray to our includes (if we are in a rush to make packages).
And in the meantime I will make a pr.

[DerThorsten]

@constantinpape the strategy you describe is not good.
It it is referred to as the "deadly diamond of death".. That would be very bad I guess =)

[constantinpape]

@DerThorsten Oh, I see.
Apparently this is what virtual inheritance in c++ is for.
But reading about the 'ddod' for 5 mins, I totally agree:
Doesn't sound like a good idea to try this....

[DerThorsten]

@constantinpape but this is for the case with virtual functions..

( and in guess for an abstract base class)

[constantinpape]

Yeah, I just read a short blogpost that explained how to circumvent some of the pitfalls of ddod using virtual inheritance (apparently you can avoid having two instances of the top-level base class this way).
Anyway the gist was still not to use it so nevermind.

[DerThorsten]

@bluescarni I just copied marray to the include.
I will ask bjoern andres if he is willing to accept a pr.
If not, i guess we keep marray in the the include

#77

[bluescarni]

Sounds good, I am closing this IR then.