4

I'm trying out the new range-v3 library (0.5.0, clang-7.1)

I'm traversing through a graph (bfs). Each node in the graph contains some vector data (std::vector<double>). While traversing through the graph, I'm trying to create a concat_view (which is concatenation of all the vectors).

I'm trying to store this concat_view as a member variable of the graph traversal class. (default_bfs_visitor from boost graph library, to be precise). So, upfront, I will not know how many vectors I'm going to encounter. I'm doing something like this.

struct bfs_visitor 
{
private:
    ranges::v3::any_view<double> mView;
public:
    template<class Graph>
    void finish_vertex (vertex_descriptor v, const Graph& g) 
    {
        auto node = g[v];
        std::vector<double>& data = dataForNode(node);
        mView = ranges::v3::concat(mView, data);
    }
};

After I'm done visiting the graph, I process the view to extract the required information.

As the type of mView changes with each concat operation, I'm unable to explicitly specify the type of mView in the declaration.

This link says there's performance hit for any_view. Is any_view the only option?

Surya
  • 1,139
  • 1
  • 11
  • 30

2 Answers2

4

You nail the issue on the head:

  • the return type of ranges::v3::concat is different so you need type-erasure (any_view e.g.).
  • type erased lazy composed ranges are a bad idea performance-wise

In your situation I would not hesitate to replace view with a reified container:

Live On Coliru

struct bfs_visitor 
{
private:
    std::vector<std::reference_wrapper<double> > mView;
public:
    template<class Graph>
    void finish_vertex (vertex_descriptor v, const Graph& g) 
    {
        auto& node = g[v];
        ranges::v3::push_back(mView, dataForNode(node));
    }
};

NOTE IMPORTANT

Note that I made auto& node a reference, instead of taking a copy. Concatenating views of temporary copies is a bad idea (UB).

If you happen to know that dataForNode(node) does NOT return a reference to member data from node, then this is not a real issue and you can disregard this comment.

PHYSIC MODE ENGAGED:

If your issue was that g is Graph const& and the view is not readonly, either

  • make mView an any_view<double const>
  • store a non-const pointer to your graph in the visitor and use that instead of the g argument

In fact, if you don't need them to be references at all (this is the key property of a view):

struct bfs_visitor 
{
private:
    std::vector<double> mCollectedData;
public:
    template<class Graph>
    void finish_vertex (vertex_descriptor v, const Graph& g) {
        ranges::v3::push_back(mCollectedData, dataForNode(g[v]));
    }
};

Note Another slightly weird thing is that your visitor templates the Graph argument type, but not the vertex_descriptor. Again, it's hard to know whether it's actually wrong without seeing the rest of the code, but it's certainly a-typical.

The vertex descriptor type is characteristic of the Graph type, so consider - writing typename Graph::vertex_descriptor or typename boost::graph_traits<Graph>::vertex_descriptor - making the operator() non-template

sehe
  • 374,641
  • 47
  • 450
  • 633
  • It certainly answered my question. The `vertex_descriptor` is indeed `typename Graph::vertex_descriptor`. – Surya May 14 '19 at 02:09
0

You can store an any_view<double>, as you wrote in your original question.

The problem was that you were trying to do a | ranges::view::concat(...) operation multiple times at runtime. Each time a range is concat-ed, it creates a different type.

Instead, you can use ranges::view::join to "flatten" the sequence of vectors of doubles, into a range of doubles. See this question, which is on point: range v3 flattening a sequence

#include <iostream>
#include <set>
#include <vector>

#include <range/v3/all.hpp>

struct graph 
{
    using node_data = std::vector<double>;
private:
    std::set<node_data> m_nodes;
    ranges::v3::any_view<double> m_view;  // Must be updated whenever a node is added/updated.

    void update_view() 
    {
        m_view = m_nodes | ranges::view::join;
    }
public:
    template<typename Container>
    graph(const Container& c) 
        : m_nodes{std::begin(c), std::end(c)}
    {
        update_view();
    }

    std::size_t size() const { return m_nodes.size(); }
    const ranges::v3::any_view<double>& data_view() const { return m_view; }
};

I tried to create an alias for the exact type of the join iterator, instead of using any_view, but I couldn't get it to work. It should be possible with decltype.

This would be used as follows (Wandbox link):

int main()
{
    using node_data = graph::node_data;
    auto data = {node_data{0.}, node_data{1.1, 2.2}, node_data{3.3, 4.4, 5.5}, node_data{6.6, 7.7, 8.8, 9.9}};

    graph g{data};
    std::cout << "Graph size (nodes): " << g.size() << std::endl;
    std::cout << "Combined data:      ";
    auto r = g.data_view();
    for (double val : r)
    {
        std::cout << val << " ";
    }
}

Output:

Graph size (nodes): 4
Combined data:      0 1.1 2.2 3.3 4.4 5.5 6.6 7.7 8.8 9.9 
NicholasM
  • 4,557
  • 1
  • 20
  • 47