fix: PI format for rust Circuit display#15917
Conversation
…ratio up to a certain limit. also added relative tests named as test_pi_float_format. files modified :crates/circuit/src/circuit_drawer.rs .This concerns the issue Qiskit#15849
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
eliarbel
left a comment
There was a problem hiding this comment.
Thanks @OnyxBrumeSky for your interest in contributing to Qiskit!
This is a very nice start, supporting multiples of
qiskit/qiskit/circuit/tools/pi_check.py
Line 27 in 919eaf6
To this end, could you please add support for these cases:
- Fractions of the form
$\frac{n \pi }{d}$ and$\frac{n}{d \pi}$ where$n \in \pm{1 \ldots 16}$ and$1 \le d \le 16$ . Note that the current implementation allows reduced fractions whose numerators are larger than 16. It also limits the denominators up to 15. - Powers of
$\pi$ , up to degree 4. -
$\frac{\pi}{n}$ form, for non-zero integers$n$ .
In addition, I've left some minor inline comments.
…meters to the function to copy the use of pi_check. Added the possibility of choosing a specific format. Next step is to add the features to support other format.
… for better reading. Added support of formats according to the original py_check.py. Added tests for the rust function. Modified the lib.rs to be able to use the function accross files.
|
Hello, I did the changes you requested and rewrite the function to follow the behavior of : qiskit/qiskit/circuit/tools/pi_check.py Line 27 in 919eaf6 I added the same comments but adapted to rust. Now the function returns an Option where Some happens when it can format Pi and None is returned if no pi formatting is found. I also moved the code in the file py_check.rs as there is a py_check.py. I also added a pub enum for formatting that accepts : Text, Qasm, Latex, Mpl. I don't know if it's really relevant here. Please let me know if there is any changes required and I will do it with pleasure. Also is there any other features/fix I could be assigned to ? |
eliarbel
left a comment
There was a problem hiding this comment.
Thanks for addressing the review comments. I'm afraid there was some misunderstanding of the intent in my earlier comment. The request behind this PR is not about fully porting the Python function pi_check to Rust but only to enable pretty-printing of floating-point angles in the Rust text drawer. So we don't need full feature parity with the Python pi_check function at this point. In addition, since the requested functionality is only needed by the Rust drawer, we should keep the new function inside circuit_drawer.rs for now.
To further clarify, I would spec the required feature as follows:
- From output formatting perspective, we should support the cases outlined here.
- The function signature should be simple. Something like that:
`fn format_float_pi(f: f64) -> Option<String>`should work (feel free to rename the function).
3. The function should reside in circuit_drawer.rs.
4. I strongly prefer to have all the new function's tests reside in a single test function (in circuit_drawer.rs), to avoid excessive blow up of mod test in the file.
Thanks for the continuous effort. I'm looking forward for the updated PR version.
|
Oh sorry for the misunderstanding. I will do the changes asap. I should finish it by today. |
… the same file. Removed function parameters as asked and modified output to only have text format
|
I just applied the changes you asked. Please tell me if anything needs to be improved . And again I am very sorry for the misunderstanding. |
eliarbel
left a comment
There was a problem hiding this comment.
Thanks again for addressing the comments and no worries, we're getting closer.
Overall, the logic seems fine to me (I assume it's an AI-generated port from pi_check, right?) I've left a couple of correctness-related suggestions (namely returning f itself if no formatting is found) and some code organization comments. I also left recommendations for more idiomatic Rust in several places. I'm fine if those are ignored for now, just please make sure you understand the generated implementation and tests.
Finally, before the next push, please run cargo clippy and cargo fmt and all the Rust tests in of the test module in this file as I would like to trigger CI checks on the PR.
…nto an array for better reading.
|
Hello, I just applied the changes. I also run cargo clippy and fmt plus the tests in the file. Let me know if any modifications are still needed. |
Thanks! This looks almost ready to go. I just pushed a commit with final touch-ups mostly around comments and the tests, before having this checked by CI. |
eliarbel
left a comment
There was a problem hiding this comment.
This is ready to merge now. Thanks @OnyxBrumeSky for your contribution!
If you'd like to contribute more to the Rust circuit drawer, feel free to pick up any remaining tasks from here: #15849. Obviously you're welcome to pick up issues in other areas.
fix: added an intermediate function format_float_pi to find closet pi ratio up to a certain limit. also added relative tests named as test_pi_float_format. files modified :crates/circuit/src/circuit_drawer.rs .
Summary
This aims to fix the pi display for rust circuit displays as mentioned in #15849 for fix nb 2. I added a function to convert floats to a ratio of pi if possible.
Details and comments
AI tool used: Claude in web browser with sonnet 4.6.
I used Claude AI to help me understand the file and where to apply my changes.
The function takes a float and try to find the closet ratio of pi possible. The smallest denominator possible is 15 and my difference delta is 1e-9. example : π/15 => "π/15" but π/16 => "0.19634954084936207". The max denominator could be increased and same for the difference delta. I also added test to my function. They are listed at the end of the tests on the page.