Add Observables#10
Conversation
* Add tests for observables * Modified arguments to 'new' so that they do not need to be mutable
emilkovacev
force-pushed
the
add-qk-obs
branch
from
00fe4f8 to
331708b
Compare
* Also moves assertions from indices and coeffs doctests into tests
|
Would it make sense to conditionally include a wrapper for this once we have #22? |
I'm happy to include it in this PR as a regular method rather than having a conditional wrapper, if that is helpful for completing the python bindings feature. |
I think, if it is to be included, it should be placed behind a feature flag to avoid depending on pyo3 unconditionally. |
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
I took a brief look at the code. It looks good but I have some comments.
| } | ||
| } | ||
|
|
||
| /// An observable over Pauli bases that stores its data in a qubit-sparse format. |
There was a problem hiding this comment.
This is sufficient for now but we should have a conversation as to how much documentation do we want to include for structures like these.
|
|
||
| #[derive(Clone, Copy, PartialEq, Debug)] | ||
| #[repr(u8)] | ||
| pub enum BitTerm { |
There was a problem hiding this comment.
This may also benefit from some documentation. I also wonder if there is a way of automatically generating the bindings for C-enumerations into Rust bindings.
I know we were having issues with that before but I wonder if we could explore that again.
There was a problem hiding this comment.
Looking at this very briefly, we could maybe implement https://docs.rs/bindgen/latest/bindgen/callbacks/trait.ParseCallbacks.html#method.item_name for bindgen in qiskit-sys? This may reduce the overhead in creating and maintaining all these enum mappings and structures.
I think this is out of scope of this PR, but happy to open an issue to investigate.
| } | ||
|
|
||
| /// A complex, double-precision number representation. | ||
| pub type Complex64 = qiskit_sys::QkComplex64; |
There was a problem hiding this comment.
This is one thing I'm not sure should be brought over as is from C. Rust has its own implementation of Complex64, we could perhaps implement From for a wrapper over this. But then again, I can see how this is utterly complex as you'd have to constantly go from one abstraction to the other, but I don't believe it is a costly operation.
There was a problem hiding this comment.
I can't seem to find an implementation of complex numbers in the rust standard library, is there an external crate typically used for this?
As for QkComplex64, my opinion is that the structure is sufficient for this use case, the rust struct generated is fairly simple:
pub struct QkComplex64 {
#[doc = "````ignore\n The real part.\n````"]
pub re: f64,
#[doc = "````ignore\n The imaginary part.\n````"]
pub im: f64,
}
Still, I'm not super familiar with rust, so if there is a standard library I'm missing, or a crate that would be more appropriate, happy to move it!
There was a problem hiding this comment.
Yeah, I misspoke. Rust doesn't have a native implementation for complex numbers. I believe that for Qiskit we used the num_complex trait to represent complex numbers.
That said, we could have methods and functions that accept complex numbers receive Complex64 and then perform the conversion internally. The purpose would be to use rust-y types instead of creating our own representations for every binding. But again, that is something we can discuss further.
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
This is about ready to merge, just a couple more comments.
3 participants
Closes #2
Progress
qk_obs_add_term(Not in-scope of this PR)qk_obs_term(Not in-scope of this PR)qk_obs_to_python