CSparseTerm & exit codes
#42
Conversation
crates/c_ext/src/exit_codes.rs
Outdated
|
|
||
| impl From<ArithmeticError> for ExitCode { | ||
| fn from(_value: ArithmeticError) -> Self { | ||
| ExitCode::ArithmeticError // do we want to cover each error enum here? |
There was a problem hiding this comment.
I am not sure I understand this comment? This would apply to the whole impl block and not just this one line, correct?
There was a problem hiding this comment.
Since ArithmeticError is an enum, we could map each enum value to a specific exit code. So
fn from(value: ArithmeticError) -> Self {
match value {
ArithmeticError::MismatchedQubits => ExitCode::MismatchedQubits,
ArithmeticError::PotentialFutureErrorThatDoesn'tExitYet => ExitCode::<...>
...
}
}right now just any enum value get's mapped to ArithmeticError. It might be nicer to have more specific error codes to look up. Given that ArithmeticError has only one possible value, we could maybe just add it.
There was a problem hiding this comment.
Oh I didn't know that, thanks 👍
| @@ -55,7 +55,7 @@ fn _accelerate(m: &Bound<PyModule>) -> PyResult<()> { | |||
| add_submodule(m, ::qiskit_accelerate::results::results, "results")?; | |||
| add_submodule(m, ::qiskit_accelerate::sabre::sabre, "sabre")?; | |||
| add_submodule(m, ::qiskit_accelerate::sampled_exp_val::sampled_exp_val, "sampled_exp_val")?; | |||
| add_submodule(m, ::qiskit_accelerate::py_sparse_observable::py_sparse_observable, "py_sparse_observable")?; | |||
| add_submodule(m, ::qiskit_accelerate::sparse_observable::py_sparse_observable, "py_sparse_observable")?; | |||
There was a problem hiding this comment.
Do we still have something called py_sparse_observable? I thought we got rid of that name 🤔
There was a problem hiding this comment.
I left the module named py_sparse_observable, but we can also rename that. Might make more sense from a Python perspective to import qiskit._accelerate.sparse_observable than qiskit._accelerate.py_sparse_observable.... 😄
| /// Errors related to C input. | ||
| #[derive(Error, Debug)] | ||
| pub enum CInputError { | ||
| #[error("Unexpected null pointer.")] | ||
| NullPointerError, | ||
| #[error("Non-aligned memory.")] | ||
| AlignmentError, | ||
| } |
There was a problem hiding this comment.
Would this make more sense to have in exit_codes or some error-specific module?
I suspect there will be more like this in the future and then it would be weird to import this from sparse_observable all the time.
There was a problem hiding this comment.
Good point, I can move this 👍🏻
| pub struct PauliTerm { | ||
| bit_term: BitTerm, | ||
| index: u32, | ||
| pub struct CSparseTerm { |
There was a problem hiding this comment.
Would it make sense to add a comment in the docstring above pointing out that this struct is exposed under SparseTerm under C?
Or actually qk_SparseTerm? (not sure how the prefix gets applied to CamelCase names... 🤔
There was a problem hiding this comment.
I hope we can prefix structs and enums with Qk and functions with qk_ 🙈 I was wondering whether doxygen is able to do renames like this, but if not, then we should use the final names here yes.
- prefixes for functions - silence some cbindgen warnings - add obs_boundaries - add clang-format to requirments
Summary
CSparseTermstructure for more flexible memory handlingDetails and comments
This also includes comments done in Qiskit#13595