-
Notifications
You must be signed in to change notification settings - Fork 147
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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)
Pull Request Test Coverage Report for Build 1595410260
💛 - Coveralls |
There was a problem hiding this 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 🚀
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 |
@Morcu I added the missing entry on documentation and your first contribution has been merged! Thanks for writing the generator |
A new generator has been added as well as it's tests.