Skip to content

Conversation

@hsunwenfang
Copy link

@hsunwenfang hsunwenfang commented Sep 6, 2025

https://github.com/Qiskit/rustworkx/issues/1496

  • I ran rustfmt locally
  • I have added the tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left many comments:

  • We control all the code, please don't panic! or unwrap. Return Result<T, E>.
  • Python tests live in tests/. We only use #[cfg(test)] in rustworkx-core. Either migrate the tests or remove them
  • For universal functions, please use _rustworkx_dispatch. We don't need Python modules for graph6, digraph6, and sparse6.

Please take a look at https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md, especially for formatting, tests, and adding type annotations.


def test_read_graph6_file(self):
"""Test reading a graph from a graph6 file."""
with tempfile.NamedTemporaryFile(mode="w", delete=False) as fd:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why the whole test couldn't exist within the with statement. I'd rather not have a os.remove statement and let tempfile deal with it

Example:

with tempfile.NamedTemporaryFile() as fd:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use tempfile now

content = f.read()
self.assertEqual(content, "C~")
finally:
os.remove(path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment for all occurrences

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

def test_write_and_read_triangle(self):
g = rx.PyGraph()
g.add_nodes_from([None, None, None])
g.add_edges_from([(0, 1, None), (1, 2, None), (0, 2, None)])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with self.assertRaises(rx.Graph6ParseError):
rx.parse_graph6_size(bad_hdr)

def test_overflow(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining this test would be great, especially bitwise manipulations

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented

self.assertEqual(g2.num_edges(), g.num_edges())


if __name__ == '__main__': # pragma: no cover
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary, you can remove it from all files. we use stestr python -m unittest also handles it AFAIK in our release

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@@ -0,0 +1,48 @@
features:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read https://docs.openstack.org/reno/latest/user/usage.html#editing-a-release-note for the correct format of release notes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used reno to generate one with nox -e docs compatible doc

src/graph6.rs Outdated
DiGraph(DiGraph),
}

let wrapped = std::panic::catch_unwind(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use std::panic::catch_unwind. If you need to return an error use, Result as a return type with Err. Not panic! or unwrap.

This will likely break with WASM.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@@ -0,0 +1,30 @@
"""digraph6 format helpers.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, this wrapper is pointless:

  • For writing, see
    def write_graphml(graph, path, /, keys=None, compression=None):
    for the correct pattern
  • For reading, if the file format contains the hint: use the same pattern from
    pub fn read_graphml<'py>(
    . If it doesn't, then just have two functions digraph_read_graph6 and graph_read_graph6

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed graph6.py, digraph6.py, sparse6.py and use a write_graph6 function in init with _rustworkx_dispatch

src/digraph6.rs Outdated

#[pyfunction]
#[pyo3(signature=(digraph, path))]
pub fn digraph_write_graph6_file(digraph: Py<crate::digraph::PyDiGraph>, path: &str) -> PyResult<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call the methods that write to file *_write_graph6, *_write_sparse6, etc. For methods that write to string, use to_str or from_str.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is now graph_write_graph6 digraph_write_graph6

@hsunwenfang
Copy link
Author

Thanks @IvanIsCoding for all the suggestion
Please feel free to point out any further things~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants