Skip to content

Conversation

@akrzemi1
Copy link
Contributor

@akrzemi1 akrzemi1 commented Jun 5, 2025

This is an initial docs page. It is very incomplete as of now, but it already offers a useful introduction to the library.

This is an initial docs page. It is very incomplete as of now,
but it already offers a useful introduction to the library.
Copy link
Contributor Author

@akrzemi1 akrzemi1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also shows how I envision each algorithm should be documented. In particular:

  • the order of the clauses (the Standard Library is strict about it),
  • the clause Constraints instead of concepts in the declarations,
  • the presence of section Throws.

@akrzemi1
Copy link
Contributor Author

After some conversations with LEWG members, it is now my understanding that it is ok for the libraries targeting the standard to specify the template constraints in the declarations, so I got rid of the Constraints: clause.

@akrzemi1
Copy link
Contributor Author

This PR addresses #149.

Copy link
Collaborator

@pratzl pratzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good start. I didn't see anything that was wrong, but a number of refinements and some different views.

| `G` | | the type of the graph that the algorithm is instantiated for |
| `vd` | `vertex_info<vertex_id_t<G>, vertex_reference_t<G>, void>` | visited vertex |
| `ed` | `edge_info<vertex_id_t<G>, true, edge_reference_t<G>, void>` | visited edge |

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing visitor functions:

  • on_initialize_vertex(vd)
  • on_examine_vertex(vd)
  • on_tree_edge(ed)
  • on_back_edge(ed)
  • on_forward_or_cross_edge(ed)
  • on_finish_edge(ed)
    See wg21.link/P3128 for descriptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to stick to the approach where the documentation describes what is present in the library rather than what is present in the paper.

  • on_initialize_vertex(vd) -- I will document it, but first I would like to understand what this one is for: Use cases for visitors #157
  • on_examine_vertex(vd) -- it is documented
  • Regarding the last four, these names do not occur even once in the graph-v2 library, at least in branch master. I would rather describe them when algorithm depth_first_search is implemented.

A number of functions in this section take a _visitor_ as an optional argument.
As different _events_, related to vertices and edges, occur during the execution of an algorithm,
a corresponding member function, if present, is called for the visitor.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some points that might useful to people.

  1. No runtime overhead occurs if the visitor function isn't defined on the visitor class.
  2. The visitor functions are the same used by boost::graph.
  3. Each algorithm defines the visitor functions they support. Additional functions included in the visitor that aren't supported are ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied #1 and #3. Not sure about #2. First, strictly speaking this cannot be true. The Boost.Graph names are without the on_ prefix. Second, I wouldn't like to give an impression that one needs to know Boost.Graph to understand this library.

The distances of each path are returned directly vie the output function argument.
The paths themselves, if requested, are only returned indirectly by providing for each vertex
its predecessor in any shortest path.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested wording for clarity:
The distances of each edge are returned directly via the output function argument. The paths themselves are returned indirectly in the predecessors vector, where each element is the preceding vertex for the matching vertex.
(My wording still feels a little awkward; just giving ideas)


The shortest paths algorithm builds on the idea that each edge in a graph has its associated _weight_.
A path _distance_ is determined by the composition of weights of edges that constitute the path.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion:
A path distance is the sum of the edge weights of the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reluctant to use word "sum" due to this example: #169.

#### Customization

1. Returns `g.vertices()`, if such member function exists and returns a `std::move_constructible` type.
2. Returns `vertices(g)`, if such function is ADL-discoverable and returns a `std::move_constructible` type.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous.


The CPO `vertex_id(g, ui)` is used obtain the _id_ of the vertex, given the iterator.
We also use its return type to determine the type of the vertex id: `vertex_id_t<G>`.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now. With the addition of descriptors, this is the only function signature that changes. Once that's available, ui becomes u (descriptor).


#### Coustomization

1. Returns `ui->vertex_id(g)`, if this expression is valid and its type is `std::move_constructible`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this will remain valid when descriptors are used. It means that vertex_id(g) would need to be defined on the descriptor, which may be possible.

#### Coustomization

1. Returns `ui->vertex_id(g)`, if this expression is valid and its type is `std::move_constructible`.
2. Returns `vertex_id(g, ui)`, if this expression is valid and its type is `std::move_constructible`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the return type is simple like an integral id then this is fine. If we want to extend the id type to be more than integral, like a user-defined type or a string, then we won't want to require move_constructible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that my goal is to document what is in the current library (modulo bugs), rather than a vision for the future.

After a bit of investigation, I conclude that the current Graph Container Interface requires the IDs to be copy_constructible!

Suppose that I have my own graph representation that needs to use non-copyable, non-movable IDs. How am I supposed to customize vertex_id?

ID const& vertex_id(Graph const& g, Graph::const_iterator it) { 
  return it->first;
}

Shall I return by value or by reference to const? If by value, then I need to copy. If by reference, then the CPO vertex_id will do the copying, because its return type is non-reference, and I am returning a reference to const, so even move will not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #174.


1. Returns `ui->vertex_id(g)`, if this expression is valid and its type is `std::move_constructible`.
2. Returns `vertex_id(g, ui)`, if this expression is valid and its type is `std::move_constructible`.
3. Returns <code>static_cast&lt;<em>vertex-id-t</em>&lt;G&gt;&gt;(ui - begin(vertices(g)))</code>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vertex_id_(g,u) is used to define vertex_id_t, so there may be a circular definition here (I hope that's not because of the current definition in graph-v2).

I think this needs to be flushed out a little more to define the vertex type based on vertex_t and whether it is a range or not (for #1 & #2 below).

@akrzemi1
Copy link
Contributor Author

akrzemi1 commented Jul 2, 2025

Thanks for the feedback!
Before I address individual points, I need to highlight some general remarks.

  1. I observed a discrepancy between what the papers describe and what is declared in this library. I resolve this conflict by documenting what I see in this library.
  2. I realize that some of this docs will become obsolete after the descriptor addition. This is why I would love to see it applied soon.
  3. I stop short before documenting things that I either do not understand or consider buggy or feel they should be improved. I often file these as issues. For instance, I thing we do not need two concepts index_adjacency_list and adjacency_list. I would like to get your feedback on:

Copy link
Collaborator

@pratzl pratzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing vd and ed as new conventions doesn't provide much value to me. It might also introduce confusion because it implies something unique. I think the use of u and uv, as is used in the papers and elsewhere, should be sufficient.

@pratzl pratzl merged commit e95bc5e into stdgraph:master Jul 24, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants