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

bfs_search, etc. have incorrect type stubs #1130

Closed
p0llard opened this issue Mar 3, 2024 · 5 comments
Closed

bfs_search, etc. have incorrect type stubs #1130

p0llard opened this issue Mar 3, 2024 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@p0llard
Copy link

p0llard commented Mar 3, 2024

pub fn graph_bfs_search(
py: Python,
graph: &graph::PyGraph,
source: Option<Vec<usize>>,
visitor: Option<PyBfsVisitor>,
) -> PyResult<()> {
shows source is an Optional<Vec<usize>> but the Python type stubs show the binding taking a single int:
def bfs_search(
graph: PyGraph | PyDiGraph,
source: int,
visitor: _BFSVisitor,
) -> None: ...

The examples in the documentation (correctly) show singleton lists being passed:

rx.bfs_search(graph, [0], vis)

If a single int is passed, then

TypeError: argument 'source': 'int' object cannot be converted to 'Sequence'

is raised.

I'm not familar enough with PyO3 to know whether it would allow implicit conversions of any Iterable[int]; this page implies it needs to be a list, but the error message (which comes from PyO3) implies it might accept collections.abc.Sequence or something similar.

@p0llard
Copy link
Author

p0llard commented Mar 3, 2024

I'm not familar enough with PyO3 to know whether it would allow implicit conversions of any Iterable[int]; this page implies it needs to be a list, but the error message (which comes from PyO3) implies it might accept collections.abc.Sequence or something similar.

I carried out a quick experiment, and it seems that arbitrary collections.abc.Sequence subclasses are indeed accepted, and work as expected.

@IvanIsCoding
Copy link
Collaborator

Because our library is written in Rust we had to manually add the type stubs.

Not only did I get bfs_search wrong, I also got dfs_search and dijkstra_search incorrectly as well. I think we can release a fix for this for sure in 0.15 and maybe it warrants a 0.14.2 release.

In the meantime you will need to ignore the annotation if you are using mypy/pyright/etc

@IvanIsCoding IvanIsCoding added bug Something isn't working good first issue Good for newcomers labels Mar 3, 2024
@JPena-code
Copy link
Contributor

Hi @IvanIsCoding.
I would like to work on this issue, if possible can you assign it to me

@IvanIsCoding
Copy link
Collaborator

Hi @IvanIsCoding. I would like to work on this issue, if possible can you assign it to me

Sounds good. Check https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md. You will need to generate a release note + change the rustworkx.pyi + init.pyi files. If you include universal and type-specific functions for Dijkstra, BFS, and DFS they will be 9 in total

@IvanIsCoding
Copy link
Collaborator

I will try to target release the fix for this with the fix for #1117 in 0.14.2

mtreinish pushed a commit that referenced this issue Mar 12, 2024
This PR is to release the fixes for #1130 and #1117. It should be a straightforward minor bugfix release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants