-
Notifications
You must be signed in to change notification settings - Fork 145
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
Conversation
Pull Request Test Coverage Report for Build 4092263202
💛 - Coveralls |
graph.update_edge( | ||
graph.from_index(source_index), | ||
graph.from_index(target_index), | ||
default_edge_weight(), | ||
); |
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 if default_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.
/// | ||
/// Arguments: | ||
/// .. note:: |
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
It will just render in the docs as plain text ... note::
without anything special (as this is a sphinx directive and the rustworkx-core docs are built using rustdoc). You can test the output with cargo doc --open
and see what the rendered docs look like: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#rustworkx-core-documentation
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.
rustworkx-core/src/generators/mod.rs
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this expose rustworkx_core::generators::mod::utils
as a public facing interface. Do we want to commit to the interface on the util functions 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.
This is just there since rustworkx/src/steiner_tree.rs
was using the pairwise
fn in generators.rs
. I moved pairwise
to utils since it was being used by a couple of the generators, but no longer used in generators.rs
. If you don't want pub here, the simplest would be to just duplicate pairwise
in steiner_tree
.
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, 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 pairwise
maybe we should just export only that one function. I'm just worried about committing the API surface for what were previously internal functions in the python side.
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.
So where should we put pairwise
so it's accessible from both rustworkx
and rustworkx-core
?
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.
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 pub use utils::pairwise
or maybe make a top level public utils module that only contains the pairwise function for now.
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.
I added a top-level utils with pairwise in it. In 5c3ea62.
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.
Overall this LGTM, thanks for fixing all this. I just left a couple of inline comments on the python side docs. The only other thing is a small change to making rustworkx_core::generators::utils
public so we limit it to just the pairwise
function.
Had a fairly major merge conflict with the pyo3 changes, which I think is all fixed, but you might want to take another look at generators.rs. |
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.
Overall I think this is mostly ready to go, just a couple small inline comments and questions. It's a bit tricky to review because the github webui shows some functions shifting positions, but I think it's all good
src/generators.rs
Outdated
/// :class:`~rustworkx.PyGraph` object will not be not be a multigraph and | ||
/// won't allow parallel edges to be added. Instead | ||
/// :param bool multigraph: When set to ``False`` the output | ||
/// :class:`~rustworkx.PyDiGraph` object will not be not be a multigraph and |
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.
The github web ui diff is a bit weird so I might be reading this wrong, but shouldn't this be PyGraph
because this is the undirected star graph variant?
/// An ASCII diagram of the graph is given by: | ||
/// | ||
/// .. code-block:: console | ||
/// .. code-block:: text |
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.
Just curious the heavy hex ascii art uses text
and heavy square uses console
, was there a difference between the two?
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.
Don't actually know why there is a difference. I did run into a problem on the rust docs side with the \
's on the heavy square drawing, but not sure that's related.
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.
LGTM, thanks for making the updates
This PR cleans up the generators documentation and makes the code for the generators more consistent. It's based on #780 and should be merged after that one.