ndarray-stats icon indicating copy to clipboard operation
ndarray-stats copied to clipboard

Remove 'static bound from type `A` in `CorrelationExt.cov`

Open LukeMathWalker opened this issue 7 years ago • 3 comments

As soon as https://github.com/bluss/ndarray/pull/491 is merged in ndarray.

LukeMathWalker avatar Sep 21 '18 07:09 LukeMathWalker

bluss/ndarray#491 is now merged.

jturner314 avatar Sep 28 '18 16:09 jturner314

I was wrong that removing the A: LinalgScalar bound on .mean_axis() would allow us to remove the need for the A: 'static bound on .cov(). The call to .dot() (let covariance = denoised.dot(&denoised.t());) has the same issue.

We can't remove the 'static bound from .dot() without tradeoffs since the .dot() implementation relies on std::any::TypeId to specialize for f32 and f64. We have two options to remove the 'static bound on .dot() by changing ndarray:

  • Don't specialize for f32 and f64. This would prevent ndarray from using BLAS.
  • Perform the specialization through LinalgScalar trait implementations. This would work but would mean that LinalgScalar could no longer be implemented for T: (the various constraints); we'd have to implement LinalgScalar for individual types. This would mean that users of ndarray and another crate other could not use .dot() with types from other unless other or ndarray provided the LinalgScalar implementation.

Alternatively, we could to modify the Rust language/standard library to either (1) allow specialization of trait implementations (an RFC has been accepted for this but hasn't been implemented yet) or (2) remove the 'static bound on std::any::TypeId.

A final alternative is to provide another .dot() method in ndarray (named .dot_any() or something) that does not perform any specialization and so doesn't require the 'static bound. We'd need to then add .cov_any() as well. This approach isn't too bad but makes the API uglier.

In conclusion, I'd prefer to just keep the A: 'static bound for the time being until (1) someone has a specific use case where the 'static bound is violated (which I expect would be quite rare) or (2) the Rust language/standard library has the necessary improvements to remove the 'static bound without the disadvantages described above.

jturner314 avatar Sep 28 '18 18:09 jturner314

I agree with the conclusions of your analysis - waiting for https://github.com/rust-lang/rust/issues/31844 to get implemented is probably the best course of action for now. Unfortunately it is not going to be part of the 2018 Edition, hence we will probably have to wait a year or so to see any tangible progress on that RFC.

LukeMathWalker avatar Sep 29 '18 14:09 LukeMathWalker