-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs and consistency cleanup for rustworkx-core generators #784
Changes from 12 commits
7b52d4b
3ed7207
85d214e
1775dff
8230812
0d88686
4fe1f17
1764ba1
815ba3f
52a2e9c
7fc6e28
12fb467
63d2307
b0e4dfc
c4d0551
4b8f1ae
5c3ea62
bef491b
663a334
b2bcf0c
cc8be50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,19 +15,18 @@ use std::hash::Hash; | |
use petgraph::data::{Build, Create}; | ||
use petgraph::visit::{Data, GraphProp, NodeIndexable}; | ||
|
||
use super::utils::pairwise; | ||
use super::utils::{get_num_nodes, pairwise}; | ||
use super::InvalidInputError; | ||
|
||
/// Generate a lollipop graph | ||
/// Generate an undirected lollipop graph where a complete graph is connected to a | ||
/// path. | ||
/// | ||
/// Arguments: | ||
/// .. note:: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this syntax will work in rustdoc since it's markdown based. I'm not sure what the correct syntax is off the top of my head, we'll have to check the docs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This passes the tests. Is there somewhere this would fail and not be tested? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will just render in the docs as plain text There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
/// | ||
/// Generate an undirected lollipop graph where a mesh graph is connected to a | ||
/// path. | ||
/// If neither `num_path_nodes` nor `path_weights` (described below) are | ||
/// specified, then this is equivalent to a complete graph. | ||
/// | ||
/// If neither `num_path_nodes` nor `path_weights` (both described | ||
/// below) are specified then this is equivalent to | ||
/// :func:`~rustworkx.generators.mesh_graph` | ||
/// Arguments: | ||
/// | ||
/// * `num_mesh_nodes` - The number of nodes to generate the mesh graph | ||
/// with. Node weights will be None if this is specified. If both | ||
|
@@ -44,8 +43,7 @@ use super::InvalidInputError; | |
/// `num_path_nodes` and `path_weights` are set `num_path_nodes` will | ||
/// be ignored and `path_weights` will be used. | ||
/// * `default_node_weight` - A callable that will return the weight to use | ||
/// for newly created nodes. This is ignored if `weights` is specified, | ||
/// as the weights from that argument will be used instead. | ||
/// for newly created nodes. This is ignored if `weights` is specified. | ||
/// * `default_edge_weight` - A callable that will return the weight object | ||
/// to use for newly created edges. | ||
/// | ||
|
@@ -98,12 +96,7 @@ where | |
if num_mesh_nodes.is_none() && mesh_weights.is_none() { | ||
return Err(InvalidInputError {}); | ||
} | ||
let num_nodes: usize; | ||
if let Some(mesh_nodes) = num_mesh_nodes { | ||
num_nodes = mesh_nodes; | ||
} else { | ||
num_nodes = mesh_weights.as_ref().unwrap().len(); | ||
} | ||
let num_nodes = get_num_nodes(&num_mesh_nodes, &mesh_weights); | ||
let num_edges = (num_nodes * (num_nodes - 1)) / 2; | ||
let mut graph = G::with_capacity(num_nodes, num_edges); | ||
|
||
|
@@ -138,8 +131,7 @@ where | |
} | ||
} | ||
}; | ||
let pathlen = path_nodes.len(); | ||
if pathlen > 0 { | ||
if !path_nodes.is_empty() { | ||
graph.add_edge( | ||
graph.from_index(meshlen - 1), | ||
graph.from_index(meshlen), | ||
|
@@ -160,6 +152,7 @@ mod tests { | |
use crate::generators::lollipop_graph; | ||
use crate::generators::InvalidInputError; | ||
use crate::petgraph::visit::EdgeRef; | ||
|
||
#[test] | ||
fn test_lollipop_mesh_path() { | ||
let expected_edge_list = vec![ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ mod path_graph; | |
mod petersen_graph; | ||
mod star_graph; | ||
|
||
mod utils; | ||
pub mod utils; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this expose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just there since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I missed that when I went through this earlier. I think that's fine but I just wonder about the rest of the utils module. If we only need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So where should we put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think we should be conservative on what we make public here. We can always add more public things later if the need arises and we're confident with the API, but it's much harder to walk something back after we make it public. I would keep the module private and change this to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a top-level utils with pairwise in it. In 5c3ea62. |
||
|
||
use std::{error::Error, fmt}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite the same because if the edge already exists we're replacing the weight with the output of
default_edge_weight()
. This won't matter ifdefault_edge_weight()
returns the same value every time, but there is no guarantee that will be the case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed that. Was thinking of the defaults as fixed. I can return to doing the find and then the add if not there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 63d2307.