undocumented filtered graph helpers?

[andrea-cassioli-maersk]

Hi,
I stumbled across some helpers in filter_graph.hpp for which I cannot find any docs:

• make_vertex_subset_filter
• make_vertex_subset_compliment_filter

and the corresponding filters

• is_in_subset
• is_not_in_subset

Are they supposed to be exposed by the library?

[Becheler]

Hi @andrea-cassioli-maersk !

Thank you for this report 👍🏽

For context, they're public convenience utilities located in <boost/graph/filtered_graph.hpp> :https://github.com/boostorg/graph/blob/develop/include/boost/graph/filtered_graph.hpp

The git history shows that they were written by Jeremy Siek (the BGL author) in 2001, as part of the original filtered_graph implementation. They were always in the boost:: namespace, never in detail:: (so de facto considered public I guess). In 2010 Jeremiah Willcock found the typo make_vertex_subset_compliment_filter but explicitly preserved the old spelling for backwards compatibility and added a note saying to use the correct one make_vertex_subset_compliment_filter. I wonder at what point that could be removed (since we bumped to C++14 anyway).

Let's ask @jeremy-murphy but I believe they are intentionally exposed helpers but undocumented because priority in early BGL was given to high-level algorithms and data structures, not trivial helpers like those.

[andrea-cassioli-maersk]

Thank @Becheler .
I also noticed the filters only support ordered sets, possibly because unordered sets where not in the stl at that time. If we keep the functionalities, it would be nice to extend the supported type (and maybe have same filters for edges?).

[Becheler]

You're right, it looks like it supports:

• std::set
• boost::unordered_set

But lacks support for:

• std::unordered_set
• boost::unordered_flat_set
• boost::container_flat_set
• generic containers with .find(), .count(), .contains()

Although I am not sure about the trade-off between code volume / utility for users. Unless I am mistaken, this is a one liner with modern C++14 lambdas:

auto g2 = make_vertex_subset_filter(graph, set);

versus:

auto g2 = filtered_graph(g, keep_all(), [&s](auto v){ return s.count(v); });

[andrea-cassioli-maersk]

I agree, a lambda is enough! So maybe the question is whether they should either documented, or removed altogether (not sure if deprecation is a thing).

[Becheler]

That looks like it could probably be removed/replaced with lambdas. They have 0 external usage outside of filtered_graph.hpp. Since they are undocumented, I doubt users heavily rely on them.

Maybe mark them as BOOST_DEPRECATED for one release, then remove them ? 🤔

[jeremy-murphy]

I will try to have a look tomorrow.

[jeremy-murphy]

Maybe I'm being old-fashioned, but sometimes it's nice to have names for things. Like, if lots of users are going to write the same lambda for a very common use case, then it probably deserves a name.

[Becheler]

I don't know. If we add make_vertex_subset_filter, the logical next request is make_vertex_property_filter, make_edge_label_filter, make_degree_filter, etc. and suddenly we have 15 variations of filtered_graph to document and maintain 😅

It seems like a tradeoff between API surface/discoverability and naming. It seems the STL algorithms solves this by defaulting to the most common use case, not coming up with new names like sort_less, sort_greater ?

Also if this is truly internal, maybe it's not a big deal :)

[jeremy-murphy]

Yes, I essentially agree and I realised I wasn't specific enough. It's not the existing names that I think are worthwhile, but the lambdas.

[Becheler]

ooooh apologies for misunderstanding, you meant that like std::less you could see a predicate family like boost::graph::label_in(set) ?

[andrea-cassioli-maersk]

IMHO either this is documented and extended to be general enough to be useful for different use cases, or it is not worth to have it the code base. As @Becheler mentioned, a simple lambda can be used, and often one needs some customisation anyway.

[andrea-cassioli-maersk]

Just implemented a node filter with a set just by a on line lambda. Works like a charm. I think we should just remove these undocumented functionalities. I can make a PR if you agree.

[Becheler]

But we don't know yet if some user relies on them ? Can we mark them as deprecated ?
PS: I guessed from recent comments on a similar issue that @jeremy-murphy would be categorizing this as technical debt, low priority, so I updated the labels, so @jeremy-murphy please tell me if I was wrong and/or fix them <3