Skip to content
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

Push generators into rustworkx-core #751

Closed
rtzoeller opened this issue Nov 25, 2022 · 5 comments
Closed

Push generators into rustworkx-core #751

rtzoeller opened this issue Nov 25, 2022 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@rtzoeller
Copy link

What is the expected enhancement?

I have a use case where I want to create graphs from Rust code, in the same way that networkx/rustworkx allows one to create graphs from Python. Unfortunately rustworkx contains the graph generators, rather than rustworkx_core, so the existing generators are not accessible to me.

I would like to see the generators abstracted of any Python-specific logic and moved into rustworkx_core.

Some quick prototyping showed something like this could work (obviously would need to think further about error handling):

// rustworkx-core/src/generators.rs

use petgraph::prelude::*;

#[inline]
fn get_num_nodes<W>(num_nodes: &Option<usize>, weights: &Option<Vec<W>>) -> usize {
    if weights.is_some() {
        weights.as_ref().unwrap().len()
    } else {
        num_nodes.unwrap()
    }
}

pub fn mesh_graph<N, E, FN, FE>(
    num_nodes: Option<usize>,
    weights: Option<Vec<N>>,
    default_node: FN,
    default_edge: FE,
) -> Result<StableGraph<N, E, Undirected>, &'static str> where FN: Fn() -> N, FE: Fn() -> E {
    if weights.is_none() && num_nodes.is_none() {
        return Err("num_nodes and weights list not specified");
    }
    let node_len = get_num_nodes(&num_nodes, &weights);
    if node_len == 0 {
        return Ok(StableGraph::<N, E, Undirected>::default());
    }
    let num_edges = (node_len * (node_len - 1)) / 2;
    let mut graph = StableGraph::<N, E, Undirected>::with_capacity(node_len, num_edges);
    match weights {
        Some(weights) => {
            for weight in weights {
                graph.add_node(weight);
            }
        }
        None => (0..node_len).for_each(|_| {
            graph.add_node(default_node());
        }),
    };

    for i in 0..node_len - 1 {
        for j in i + 1..node_len {
            let i_index = NodeIndex::new(i);
            let j_index = NodeIndex::new(j);
            graph.add_edge(i_index, j_index, default_edge());
        }
    }

    Ok(graph)
}
// src/generators.rs

#[pyfunction(multigraph = true)]
#[pyo3(text_signature = "(/, num_nodes=None, weights=None, multigraph=True)")]
pub fn mesh_graph(
    py: Python,
    num_nodes: Option<usize>,
    weights: Option<Vec<PyObject>>,
    multigraph: bool,
) -> PyResult<graph::PyGraph> {
    let graph = rustworkx_core::generators::mesh_graph::<PyObject, PyObject, _, _>(
        num_nodes,
        weights,
        || py.None(),
        || py.None(),
    );
    return match graph {
        Ok(g) => Ok(graph::PyGraph {
            graph: g,
            node_removed: false,
            multigraph,
            attrs: py.None(),
        }),
        Err(s) => Err(PyIndexError::new_err(s)),
    };
}

Alternatively I can re-implement these generators in my own application directly on top of petgraph, but that seems less than ideal.

@mtreinish mtreinish added enhancement New feature or request good first issue Good for newcomers labels Nov 28, 2022
@mtreinish
Copy link
Member

I think this is a good idea, having the generators be part of the rustworkx-core makes a lot of sense, I think your sketch of how to go about separating out a generic rust part from the python specific version makes sense. I am wondering if we can go a step further making it generic with Build and Create traits and not have it always return a StableGraph.

mtreinish added a commit to mtreinish/retworkx that referenced this issue Dec 10, 2022
This commit migrates the cycle-graph generator to rustworkx-core. It
makes the function generic so that it can be used with any petgraph
graph that's buildable and node indexable. This is primarily to serve as
a pattern for migrating all the generator functions we currently have to
rustworkx-core. This pattern should be usable for any generator function
we have.

Part of Qiskit#751
@mtreinish
Copy link
Member

I pushed up #758 to prototype how to make the generator functions generic in rustworkx-core (using cycle_graph, it's about 95% there (the only missing piece is error handling). But I think we can use that as a model and quickly iterate over all the generator functions and migrate them to rustworkx-core after it merges.

mtreinish added a commit to mtreinish/retworkx that referenced this issue Dec 12, 2022
This commit migrates the cycle-graph generator to rustworkx-core. It
makes the function generic so that it can be used with any petgraph
graph that's buildable and node indexable. This is primarily to serve as
a pattern for migrating all the generator functions we currently have to
rustworkx-core. This pattern should be usable for any generator function
we have.

Part of Qiskit#751
mergify bot pushed a commit that referenced this issue Jan 5, 2023
* Move cycle_graph generator to rustworkx-core

This commit migrates the cycle-graph generator to rustworkx-core. It
makes the function generic so that it can be used with any petgraph
graph that's buildable and node indexable. This is primarily to serve as
a pattern for migrating all the generator functions we currently have to
rustworkx-core. This pattern should be usable for any generator function
we have.

Part of #751

* Apply suggestions from code review

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>

* Run cargo fmt

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
@mtreinish
Copy link
Member

@enavarro51 is there anything left for this or can we close this as complete now?

@enavarro51
Copy link
Contributor

All the ones that were in the original src/generators.rs have been ported to rustworkx-core along with the random graph generators. So I think it's fine to close this.

@mtreinish
Copy link
Member

Ok,then im going to close this as complete, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants