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

Change DAGCircuit::collect_runs() return to not be an Option #13109

Closed
mtreinish opened this issue Sep 6, 2024 · 10 comments · Fixed by #13222
Closed

Change DAGCircuit::collect_runs() return to not be an Option #13109

mtreinish opened this issue Sep 6, 2024 · 10 comments · Fixed by #13222
Assignees
Labels
good first issue Good for newcomers Rust This PR or issue is related to Rust code in the repository

Comments

@mtreinish
Copy link
Member

mtreinish commented Sep 6, 2024

It looks like DAGCircuit::collect_runs returns an Option only because it directly returns the result of the Rustworkx core function, which uses None to represent a cycle in the graph (...which is questionable lol).

Perhaps it is beyond the scope of this PR, but it might be nice if you can change DAGCircuit::collect_runs to unwrap that option internally. If it fails, it should be on DAGCircuit for getting itself into an invalid state.

Originally posted by @kevinhartman in #13013 (comment)

To fix this issue we should update the collect_runs() method in crates/circuit/src/dag_circuit.rs to internally unwrap the return from rustworkx and not return an option.

@mtreinish mtreinish added good first issue Good for newcomers Rust This PR or issue is related to Rust code in the repository labels Sep 6, 2024
@github-project-automation github-project-automation bot moved this to Tagged but unassigned in Contributor Monitoring Sep 6, 2024
@tonmoy-b
Copy link

tonmoy-b commented Sep 8, 2024

Hi, while I'm working on another issue I would love to work on this one as well. If it's ok to assign a second issue to me, kindly do so. Thanks

@AngeloDanducci
Copy link
Contributor

@tonmoy-b assigned, thanks!

@cameron-d28
Copy link
Contributor

i have created a pull request for this issue. please let me know if formatting is correct and perhaps how to test better - this is my first qiskit issue and not too familiar with rust.

@tonmoy-b
Copy link

i have created a pull request for this issue. please let me know if formatting is correct and perhaps how to test better - this is my first qiskit issue and not too familiar with rust.

Hi Cameron, someone will review your PR code but I think you need to sign the CLA first. I'm not sure how to transfer the assignee tag to you but maybe that can be done later. You probably should sign the CLA for now.

@cameron-d28
Copy link
Contributor

i just signed it - thank you for the heads up.

@cameron-d28
Copy link
Contributor

any suggestion on my pull request? happy for feedback.

@cameron-d28
Copy link
Contributor

is it okay that some of the workflows are failing?

@tonmoy-b
Copy link

is it okay that some of the workflows are failing?

Make sure you run tox, tox -eblack and tox -elint in your local system before you push. The last in particular will give you code formatting errors that you should correct so there's no issues later on

@tonmoy-b
Copy link

any suggestion on my pull request? happy for feedback.

I need till this weekend or Monday, I'll get back to you then but maybe one of the project maintainers will give a review before that.

@cameron-d28
Copy link
Contributor

cameron-d28 commented Sep 26, 2024

do you know how long it may take for pull request to be approved or to get feedback? it has just passed all the tests. @tonmoy-b @AngeloDanducci

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Rust This PR or issue is related to Rust code in the repository
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants