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

Adding full rary tree generator #463

Merged
merged 13 commits into from
Dec 18, 2021
Merged

Conversation

Morcu
Copy link
Contributor

@Morcu Morcu commented Oct 14, 2021

A new generator has been added as well as it's 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 doing this! We're up to a good start, I made comments about mostly about Rust details, I think the generator logic is correct.

src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
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.

Thanks for pushing this, I haven't looked at the code in detail yet but from a quick check could you add a release note for the new feature (the procedure is documented here: https://github.com/Qiskit/retworkx/blob/main/CONTRIBUTING.md#release-notes) and also add the function to the api documentation here: https://github.com/Qiskit/retworkx/blob/main/docs/source/api.rst#generators so the docs are rendered

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.

More Rust comments, I think we're on a good track. Also: you forgot to run cargo fmt on your code, that is why CI is failling (check the CONTRIBUTING.md file Matthew linked)

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

coveralls commented Oct 25, 2021

Pull Request Test Coverage Report for Build 1595410260

  • 48 of 48 (100.0%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 98.45%

Files with Coverage Reduction New Missed Lines %
retworkx-core/src/min_scored.rs 4 77.78%
Totals Coverage Status
Change from base Build 1594188537: -0.03%
Covered Lines: 11052
Relevant Lines: 11226

💛 - Coveralls

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, just some minor details about removing commented out code and a minor docstring detail.

Apart from that, it should be good to go after the rebase 🚀

src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
@IvanIsCoding
Copy link
Collaborator

Also, this almost slipped through but we need to add the new generator to the documentation! Check https://github.com/Qiskit/retworkx/pull/471/files#diff-b83b53d2e790a152ae629a3756c2232706f9a9660ed385196b17f63362df2c87R176 for an example, you need to modify api.rst to include your new generator

@IvanIsCoding IvanIsCoding merged commit 44a1598 into Qiskit:main Dec 18, 2021
@IvanIsCoding
Copy link
Collaborator

@Morcu I added the missing entry on documentation and your first contribution has been merged! Thanks for writing the generator

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.

4 participants