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

Make API uniform #215

Closed
eddieschoute opened this issue Dec 5, 2020 · 1 comment · Fixed by #241
Closed

Make API uniform #215

eddieschoute opened this issue Dec 5, 2020 · 1 comment · Fixed by #241
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@eddieschoute
Copy link

What is the expected enhancement?

Currently, many methods require prefixing the name with the type of the graph (i.e. digraph_). Would it be possible to dispatch the right method based on the type of the graph automatically? This arose in a discussion on a prior PR (Qiskit/qiskit#5471 (comment)).

One pythonic way to implement this would be to switch on the type, if implementing this in Rust is a problem. Then the right Rust method may be called by the Python code.

@mtreinish mtreinish added enhancement New feature or request good first issue Good for newcomers labels Dec 5, 2020
@mtreinish
Copy link
Member

I think that using python would be the easier path forward here. I've tried to do this on the rust side several ways and the issue I have is that python binding macros #[pyfunction] which generate the functions which wraps the retworkx functions and actually passes the objects from python aren't able to tell us what traits the python object it passes to the rust definitions has implemented, just that it implements FromPyObject so we get a compiler type error (that's pretty cryptic because it originates in the expanded macro generated function). It'll be easier to just do something like:

def dfs_edges(self, graph, source=None):
    """Get edge list in depth first order

    :param (PyGraph|PyDiGraph|PyDAG) graph: The graph to get the DFS edge list from
    :param int source: An optional node index to use as the starting node
        for the depth-first search. The edge list will only return edges in
        the components reachable from this index. If this is not specified
        then a source will be chosen arbitrarly and repeated until all
        components of the graph are searched.
    :returns: A list of edges as a tuple of the form ``(source, target)`` in
        depth-first order
    :rtype: EdgeList
    """
    if isinstance(graph, PyGraph):
       graph_dfs_edges(graph, source)
    else:
       digraph_dfs_edges(graph, source)

in the main package __init__: https://github.com/Qiskit/retworkx/blob/master/retworkx/__init__.py
and just do that for every split function in the api. Although, given the number of these there will be though (since it's most of the algorithm functions, all the generators, and all the random graph functions) it might make more sense to create a new ufunc.py or something like that and do this in there and then just re-export all of them from the package root.

mtreinish added a commit to mtreinish/retworkx that referenced this issue Jan 25, 2021
In the rust generated Python API we need to have fixed class inpurts to
satisfy the traits used by the pyo3 macro generated FFI functions. This
results in duplicate methods like digraph_dfs_edges and graph_dfs_edges
with the same implementation just differing input types. To simplify the
API for users this commit adds universal functions to the python side of
the retworkx package to take in any retworkx graph object and dispatch to
the proper function in the rust generated api that relies on strict
input types.

Fixes Qiskit#215
@mtreinish mtreinish mentioned this issue Jan 25, 2021
1 task
@mtreinish mtreinish self-assigned this Feb 1, 2021
mtreinish added a commit that referenced this issue Feb 3, 2021
* Add universal methods

In the rust generated Python API we need to have fixed class inpurts to
satisfy the traits used by the pyo3 macro generated FFI functions. This
results in duplicate methods like digraph_dfs_edges and graph_dfs_edges
with the same implementation just differing input types. To simplify the
API for users this commit adds universal functions to the python side of
the retworkx package to take in any retworkx graph object and dispatch to
the proper function in the rust generated api that relies on strict
input types.

Fixes #215

* Add release notes

* Cleanup docs for new functions

* Add as_undirected flag to distance_matrix()
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.

2 participants