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

Add reverse inplace function for digraph #853

Merged

Conversation

matanco64
Copy link
Contributor

I added a reverse function for a graph, it takes all the edges and reverses them,

also added tests and documentation for the function

@CLAassistant
Copy link

CLAassistant commented Apr 2, 2023

CLA assistant check
All committers have signed the CLA.

@matanco64 matanco64 force-pushed the add_reverse_inplace_function_for_digraph branch 2 times, most recently from 433044e to 7294d0f Compare April 3, 2023 07:26
@coveralls
Copy link

coveralls commented Apr 3, 2023

Pull Request Test Coverage Report for Build 4924945942

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 97.093%

Totals Coverage Status
Change from base Build 4919724815: 0.009%
Covered Lines: 14397
Relevant Lines: 14828

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Overall this looks great. I think your approach here makes the most sense to do an inplace edge reversal. I left a couple of small inline comments, the thing blocking CI I expect is my comment about the docs. The other thing which is missing is a release note to document the addition of the new feature. If you don't mind could you add one, the procedure for doing this is documented here: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#release-notes

src/digraph.rs Outdated Show resolved Hide resolved
graph.reverse_inplace()
self.assertEqual([(1, 0), (2, 1), (2, 0), (3, 2), (3, 0)], graph.edge_list())

def test_reverse_large_graph(self):
Copy link
Member

Choose a reason for hiding this comment

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

Two small test cases that I think would be good to add is one with an empty graph. The other is for a graph where there is an edge removed in the middle (which will leave a hole in the edge indices). I don't expect there are any problems with the method's behavior with these, but they're common edge cases that are good to verify (because a lot of other methods have had issues in the past with both).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Sorry for the late response and update, I've added the tests

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.

Thanks for contributing this. Overall LGTM, I just have one suggestion. Also, I second Matthew's comment that we should add some extra tests

My main suggestion is naming the method reverse because NetworkX does it that way. It is not a rule in the project but it does avoid people asking what the equivalent method is. For now, we can ignore the copy keyword and just always do it in place.

src/digraph.rs Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
tests/rustworkx_tests/digraph/test_edges.py Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
tests/rustworkx_tests/digraph/test_edges.py Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
@matanco64 matanco64 force-pushed the add_reverse_inplace_function_for_digraph branch from b071f44 to 805f7d6 Compare May 8, 2023 20:47
Copy link
Contributor Author

@matanco64 matanco64 left a comment

Choose a reason for hiding this comment

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

I've gone over all the comments

  1. renamed
  2. added tests
  3. changed to unwraps

I also added code to the interface at digraph.pyi so it will be easier to develop in pycharm and such

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.

LGTM, you just need to run cargo fmt because you forgot but it should be good to go

@matanco64
Copy link
Contributor Author

Thank You! ran cargo fmt, should be good now :)

@IvanIsCoding IvanIsCoding added automerge Queue a approved PR for merging and removed automerge Queue a approved PR for merging labels May 9, 2023
@IvanIsCoding
Copy link
Collaborator

You’ll need to fix your docstring too, it’s giving Inline literal start-string without end-string. when building the docs

@mtreinish mtreinish added this to the 0.13.0 milestone May 10, 2023
@mtreinish mtreinish added the automerge Queue a approved PR for merging label May 10, 2023
@mtreinish
Copy link
Member

I pushed up a quick fix to update the docs syntax here to get this merged as it LGTM besides the docs failure. The underlying issue was the docstring for rustworkx. The docstrings get exported to python as rst that sphinx will need to read but the docs were constructed using rust doc so I just converted that to use python/sphinx rst.

@mergify mergify bot merged commit a16c18d into Qiskit:main May 10, 2023
IvanIsCoding pushed a commit to IvanIsCoding/rustworkx that referenced this pull request May 12, 2023
* added a reverse_inplace function in digraph,

the function reverses the direction of the edges in the digraph
implemented by switching the indices of the nodes in an edge.

* added python tests for the reverse_inplace function.

testing a simple case and a case for a large graph.

* ran rust fmt and clippy, also added more detailed documentation

* rename reverse_inplace to reverse

* change excepts to unwraps (If this fails is because of PyO3. It panics and there is not much point in printing a message)

* added tests for empty graph and graph with node removed in the middle

* added interface signature for IDEs

* ran cargo fmt

* Fix doc syntax

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
mergify bot pushed a commit that referenced this pull request May 12, 2023
* Add tests from the example

* Fix bug

* Fix tests

* Add release note

* Update release note

* Apply suggestions from code review

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Fix docs to work with Sphinx Theme 1.11 (#867)

* Fix docs to work with Sphinx Theme 1.11

* Update docs/source/_templates/sidebar.html

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Turn off CI for forks (#868)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Fix pickle/deepcopy not preserve original edge indices (#589)

* fix issue #585 that pickling graph & digraph do not preserve original edge index

* fix clippy lints - collapsible_else_if

* Simplify logic in __setstate__

* Add release note

* Fix lint

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Bump serde from 1.0.160 to 1.0.162 (#863)

Bumps [serde](https://github.com/serde-rs/serde) from 1.0.160 to 1.0.162.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.160...1.0.162)

---
updated-dependencies:
- dependency-name: serde
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Add reverse inplace function for digraph (#853)

* added a reverse_inplace function in digraph,

the function reverses the direction of the edges in the digraph
implemented by switching the indices of the nodes in an edge.

* added python tests for the reverse_inplace function.

testing a simple case and a case for a large graph.

* ran rust fmt and clippy, also added more detailed documentation

* rename reverse_inplace to reverse

* change excepts to unwraps (If this fails is because of PyO3. It panics and there is not much point in printing a message)

* added tests for empty graph and graph with node removed in the middle

* added interface signature for IDEs

* ran cargo fmt

* Fix doc syntax

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Bump serde from 1.0.162 to 1.0.163 (#869)

Bumps [serde](https://github.com/serde-rs/serde) from 1.0.162 to 1.0.163.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.162...v1.0.163)

---
updated-dependencies:
- dependency-name: serde
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Extend fixes to add_edges_from and add_edges_from_no_data

* Lower amount of nodes in test

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Binh Vu <binh-vu@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: matanco64 <38103422+matanco64@users.noreply.github.com>
IvanIsCoding added a commit to IvanIsCoding/rustworkx that referenced this pull request May 26, 2023
* Add tests from the example

* Fix bug

* Fix tests

* Add release note

* Update release note

* Apply suggestions from code review

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Fix docs to work with Sphinx Theme 1.11 (Qiskit#867)

* Fix docs to work with Sphinx Theme 1.11

* Update docs/source/_templates/sidebar.html

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Turn off CI for forks (Qiskit#868)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Fix pickle/deepcopy not preserve original edge indices (Qiskit#589)

* fix issue Qiskit#585 that pickling graph & digraph do not preserve original edge index

* fix clippy lints - collapsible_else_if

* Simplify logic in __setstate__

* Add release note

* Fix lint

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Bump serde from 1.0.160 to 1.0.162 (Qiskit#863)

Bumps [serde](https://github.com/serde-rs/serde) from 1.0.160 to 1.0.162.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.160...1.0.162)

---
updated-dependencies:
- dependency-name: serde
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Add reverse inplace function for digraph (Qiskit#853)

* added a reverse_inplace function in digraph,

the function reverses the direction of the edges in the digraph
implemented by switching the indices of the nodes in an edge.

* added python tests for the reverse_inplace function.

testing a simple case and a case for a large graph.

* ran rust fmt and clippy, also added more detailed documentation

* rename reverse_inplace to reverse

* change excepts to unwraps (If this fails is because of PyO3. It panics and there is not much point in printing a message)

* added tests for empty graph and graph with node removed in the middle

* added interface signature for IDEs

* ran cargo fmt

* Fix doc syntax

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Bump serde from 1.0.162 to 1.0.163 (Qiskit#869)

Bumps [serde](https://github.com/serde-rs/serde) from 1.0.162 to 1.0.163.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.162...v1.0.163)

---
updated-dependencies:
- dependency-name: serde
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Extend fixes to add_edges_from and add_edges_from_no_data

* Lower amount of nodes in test

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Binh Vu <binh-vu@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: matanco64 <38103422+matanco64@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants