Have `CustomOperation::to_matrix` return `Result<T, E>` instead of `Option<T>`.

[davidfcohen]

What should we add?

The trait member CustomOperation::matrix should be fallible. Also, to_matrix is a more appropriate name for this function.

Potential Errors

• matrix exceeds the system size limit
• number of qubits is 0
• to_matrix is unimplemented

Example

pub trait CustomOperation {
    // Use boxed error because a generic error causes problems downstream.
    fn to_matrix(&self) -> Result<Array2<Complex64>, Box<dyn error::Error>>;
}

[Cryoris]

This seems like a nice addition -- previously we only had to distinguish into "there's a matrix" (Some(..)) or "no matrix" (None). But if there's an error in the calculation or size limits are exceeded we need more fine-grained resolving, and NoImplementation or something can still be an option.

I'm wondering if we don't want to extend this to all to_matrix methods and introduce a proper trait for it while at it?

[davidfcohen]

I'm wondering if we don't want to extend this to all to_matrix methods and introduce a proper trait for it while at it?

@Cryoris Can you elaborate?

[alexanderivrii]

Thanks @davidfcohen. I agree that having to_matrix method return Result is better than return Option (especially considering your implementation in #16809, and your comment on #16838). One quick question about your comment "Use boxed error because a generic error causes problems downstream."

The following would be simpler:

pub trait CustomOperation {
    // MatrixError from #16809
    fn to_matrix(&self) -> Result<Array2<Complex64>, CustomOperationMatrixError>; 
}

but you think this is not flexible enough?

[davidfcohen]

but you think this is not flexible enough?

@alexanderivrii The implementation should have control over the error type because different custom operations can fail for different reasons. If we mandate an error type, it would probably evolve into something a bit strange:

// halfway, might as well be Box<dyn error::Error>
enum CustomOperationMatrixError {
    Unimplemented,
    ExceedsLimit,
    Custom(Box<dyn error::Error>),
}

or

// multiple meanings, potential maintenance burden
enum CustomOperationMatrixError {
    Unimplemented,
    ExceedsLimit,
    PauliEvolution(PauliEvolutionError),
    QftGate(QftGateError),
    ...
}

I would have liked to return Self::Err as part of the trait definition, much like std::str::FromStr does. However, because of the way CustomOperation is used dynamically, I opted for Box<dyn std::error::Error> instead.

[jakelishman]

Just for consideration:

• Result<Option<Array2>, E> can be valid modelling for when Err(E) represents "some error in calculation that must be propagated" and Ok(None) is a non-exceptional state. Typically in Qiskit, "no matrix" is expected to be handled as regular logic by almost all callers, but all other errors propagate. Use sites often become it let Some(m) = ob.try_matrix()? { ... };

• This sort of modelling:

  // halfway, might as well be Box<dyn error::Error>
  enum CustomOperationMatrixError {
      Unimplemented,
      ExceedsLimit,
      Custom(Box<dyn error::Error>),
  }

  is actually pretty common in Qiskit and many places. There are lots of reasons you might choose to do it, including making frequent cases cheaper to handle and making it so there are some inspectable variants that might be specifically dispatched on. I'd argue that if your options actually are Unimplemented, ExceedsLimit and Custom(...) then that model is better than forcing Unimplemented to require a heap allocation and erasing its type.

  (That said, the Result<Option<T>, E> thing is also a choice, and arguable ExceedsLimit needn't exist - the num_qubits is always known to Qiskit PackedInstruction, and anything requesting a matrix already needs to manually check before asking an exponential calculation anyway.)