Add qk_circuit_draw function to the C API#15361
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 19696786623Details
💛 - Coveralls |
|
|
||
| uint32_t qubits[10] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; | ||
| double params[3] = {1.41, 2.71, 3.14}; | ||
| for (uint8_t gate = 0; gate <= QkGate_RC3X; ++gate) { |
There was a problem hiding this comment.
is there a simpler way to loop over all standard gates?
also, there is a gate that needs 4 parametrs (CUGate), and here only 3 are given.
should we also add reset to the test?
what about the new gates like PPM and PPR ?
There was a problem hiding this comment.
This is merely a smoke test type of thing, just to have evidence that it's working. Thanks to Matthew we now have more rigor Rust test now for the Rust text drawer.
Good catch about the 4-param gate and the missing reset instruction. Addressed in c2ce88c.
There was a problem hiding this comment.
Regarding PPR/PPM: not all gates are supported yet by the Rust drawer. We'll add more as we go.
jakelishman
left a comment
There was a problem hiding this comment.
If this is still intended to merge for the 2.4 cycle, can you give the new C functions export slots somewhere suitable in qiskit-cext-vtable?
afc2312 to
11e02ad
Compare
|
This is now rebased on top of |
Good idea. Done in 11e02ad |
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Eli! I experimented with this PR earlier, drawing circuits using the C interface, and all the gates appear correct.
jakelishman
left a comment
There was a problem hiding this comment.
I think there was a slightly mistaken merge-conflict resolution
| /// The configuration options for the ``qk_circuit_draw`` function. | ||
| #[repr(C)] | ||
| pub struct CCircuitDrawerConfig { | ||
| /// If `true`, bundles classical registers into single wires. | ||
| bundle_cregs: bool, | ||
| /// If `true`, merges the bottom and top lines of adjacent wires. | ||
| merge_wires: bool, | ||
| /// Sets the line length for wrapping the rendered text. Use 0 | ||
| /// to auto-detect console width. | ||
| fold: usize, | ||
| } |
There was a problem hiding this comment.
It's too late to change now for the release, but given this configuration may well expand in the future, we might be in a bit of an awkward position with API stability and require a breaking change - having the struct members all be visible to C makes it harder for us to modify it in the future without an API break.
We can revisit in the future if/when we need to change it, and we're explicitly not API stable yet in the C API, so no worries.
Fwiw (and this is really minor) - when in repr(C), the order of the struct members is guaranteed to be in declaration order, but then to satisfy alignment concerns this means that there's going to be padding bytes between merge_wires and fold.
There was a problem hiding this comment.
having the struct members all be visible to C makes it harder for us to modify it in the future without an API break.
I didn't want to add more functions to the API for this (i.e. getter and setters for the options if this is opaque), so I thought maybe ABI stability should be enough (since it's passed by address). Anyway, Ok, I'd be happy to have it more API-stability friendly.
Thanks about the repr(c).
There was a problem hiding this comment.
It's passed by address, but given the access pattern is meant to be (judging by the test) that the caller allocates one on the stack themselves, then the trouble will be that if we extend the struct in a later version of the C API, code compiled against Qiskit 2.4 will have only allocated the 16 bytes needed for this struct, but Qiskit 2.5 might then try to read more than 16 bytes through the pointer it's given, which is an overread error, and it'll get random stack data instead.
jakelishman
left a comment
There was a problem hiding this comment.
Thanks for this, Eli. I think given more time I might have wanted to think about the configuration API more, but given we're explicitly unstable in the C API, I'd rather get this out like this than not at all.
| for (uint8_t gate = 0; gate <= QkGate_RC3X; ++gate) { | ||
| qk_circuit_gate(circuit, gate, &qubits[gate % 8], params); | ||
| } |
There was a problem hiding this comment.
hahaha oh wow, that's quite the strategy
There was a problem hiding this comment.
It's C after all, right? 😉
There was a problem hiding this comment.
I was also surprized that there was no API like
for gate in StandardGateList
or similar...
There was a problem hiding this comment.
I mean, this basically is the way of doing it in C, but yeah, we don't have it in Rust.
There was a problem hiding this comment.
Yeah we do :p for gate in (0..STANDARD_GATE_SIZE).map(|x| unsafe { std::mem::transmute(x) }) (probably missing a type hint, but I'm on my phone)
| merge_wires: bool, | ||
| /// Sets the line length for wrapping the rendered text. Use 0 | ||
| /// to auto-detect console width. | ||
| fold: usize, |
There was a problem hiding this comment.
Do we not have an option for no folding in the api? In python we set this to -1 to not fold. I guess usize::max is fine as an equivalent. But maybe we should document that.
There was a problem hiding this comment.
I haven't really thought about this use case initially, but I can see now how this could be useful e.g. for saving the output to a file. The Rust drawer itself uses usize for the wrapping length, so using -1 as a sentinel value seems maybe too intrusive change for this, since usize::MAX practically provides this behavior. I'll update the documentation during the RC period.
This PR adds support for circuit text drawing through the C API.
It is based and blocked on #15357. See commit fefdd03 for the changes of this PR only.