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.

Sign up for GitHub

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

Jump to bottom

add find_predecessor_node_by_edge. #756

Merged
merged 15 commits into from
Dec 13, 2022
Merged

add find_predecessor_node_by_edge. #756

merged 15 commits into from
Dec 13, 2022

Conversation

ewinston
Copy link
Contributor

@ewinston ewinston commented Dec 1, 2022

add function to get immediate predecessor of node by edge.

mtreinish
mtreinish reviewed Dec 6, 2022
View reviewed changes
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 I think this looks good, thanks for writing this. The only thing I think missing (besides the two nits on the docstring inline) is a negative test that tests we raise the appropriate error if there is no matching inbound edge found

src/digraph.rs Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 6, 2022 •
edited

Pull Request Test Coverage Report for Build 3688141711

  • 21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 96.929%
Totals Coverage Status
Change from base Build 3686610686: 0.02%
Covered Lines: 13350
Relevant Lines: 13773
💛 - Coveralls

ewinston and others added 3 commits December 6, 2022 20:27
@ewinston @mtreinish
Update src/digraph.rs
1f756fc 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston @mtreinish
Update src/digraph.rs
7346a7a 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston
add negative test
0e99334
@ewinston
Copy link
Contributor Author

ewinston commented Dec 7, 2022

@mtreinish Thanks for the tip on how to implement this.

@ewinston ewinston changed the title [WIP] add find_adjacent_predecessor_node_by_edge. add find_adjacent_predecessor_node_by_edge. Dec 7, 2022
@ewinston ewinston mentioned this pull request Dec 7, 2022
Merged
mtreinish
mtreinish reviewed Dec 8, 2022
View reviewed changes
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.

The code LGTM now thanks. I just left inline comments to remove "adjacent" from the function name. I think it's a bit redundant with predecessor in the name already

The other thing I think we should have is a reno release note documenting the new feature.

src/digraph.rs Outdated Show resolved Hide resolved
tests/rustworkx_tests/digraph/test_edges.py Outdated Show resolved Hide resolved
tests/rustworkx_tests/digraph/test_edges.py Outdated Show resolved Hide resolved
tests/rustworkx_tests/digraph/test_edges.py 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
ewinston and others added 7 commits December 13, 2022 09:37
@ewinston @mtreinish
Update src/digraph.rs
a4871d5 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston @mtreinish
Update tests/rustworkx_tests/digraph/test_edges.py
bb62a02 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston @mtreinish
Update tests/rustworkx_tests/digraph/test_edges.py
1eb42bd 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston @mtreinish
Update tests/rustworkx_tests/digraph/test_edges.py
90cc724 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston @mtreinish
Update tests/rustworkx_tests/digraph/test_edges.py
2e928b5 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston @mtreinish
Update src/digraph.rs
ea89cfb 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ewinston
add reno note
93270e0
@mtreinish mtreinish added the automerge Queue a approved PR for merging label Dec 13, 2022
@ewinston ewinston changed the title add find_adjacent_predecessor_node_by_edge. add find_predecessor_node_by_edge. Dec 13, 2022
@mergify mergify bot merged commit b8360df into Qiskit:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtreinish mtreinish mtreinish approved these changes

Assignees
No one assigned
Labels
automerge Queue a approved PR for merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ewinston @coveralls @mtreinish