Better traits #24
Better traits #24
Conversation
Types types types. What type of trouble do I make for myself
Look familiar? Found it was the most ergonomic way to handle all these types
Now just to refactor the tests and the new types will be ready for review
| C: ColourModel, | ||
| { | ||
| /// Create an image given an existing ndarray | ||
| pub fn from_array(data: ArrayBase<T, Ix3>) -> Self { |
There was a problem hiding this comment.
removed in another PR built on top of this since I realised I'd duplicated from_data
| use std::{fmt, hash, marker::PhantomData}; | ||
|
|
||
| pub type Image<T, C> = ImageBase<OwnedRepr<T>, C>; | ||
| pub type ImageView<'a, T, C> = ImageBase<ViewRepr<&'a T>, C>; |
There was a problem hiding this comment.
Shame rust-lang/rfcs#2515 hasn't landed yet - it would have been nicer to phrase these as:
pub type Image<T, C> = ImageBase<impl DataMut<T>, C>;
pub type ImageView<'a, T, C> = ImageBase<impl Data<&'a T>, C>;
There was a problem hiding this comment.
Yeah, there are also bits of the code that would be nicer with const generics as well, I've just accepted eventual refactoring for the new things I want to use
| fn pad(&self, image: ArrayView3<T>, padding: (usize, usize)) -> Array3<T>; | ||
| fn pad( | ||
| &self, | ||
| image: ArrayView<T, Ix3>, |
There was a problem hiding this comment.
I think this is too restrictive - wouldn't it be more convenient to take &ArrayBase<U, Ix3> where U: Data<Elem = T>? Is there any specific reason you are using a view?
There was a problem hiding this comment.
So I tried to do it but the generics got a bit hairy especially with the convolution extension trait. Just tried to do it again and got overflow evaluating the requirement `<ndarray::ArrayBase<U, ndarray::dimension::dim::Dim<[usize; 3]>> as processing::conv::ConvolutionExt<S>>::Data. It's something I'd like to do but it's a bit of a pain (or the easy way to do it isn't yet evident to me)
src/processing/conv.rs
Outdated
| where | ||
| U: Data<Elem = T> + DataMut<Elem = T>, |
src/processing/conv.rs
Outdated
| where | ||
| U: Data<Elem = T> + DataMut<Elem = T>, |
There was a problem hiding this comment.
U: DataMut<Elem = T> should be enough.
src/processing/sobel.rs
Outdated
| fn get_edge_images<T>(mat: &Array3<T>) -> Result<(Array3<T>, Array3<T>), Error> | ||
| fn get_edge_images<T, U>( | ||
| mat: &ArrayBase<U, Ix3>, | ||
| ) -> Result<(ArrayBase<OwnedRepr<T>, Ix3>, ArrayBase<OwnedRepr<T>, Ix3>), Error> |
There was a problem hiding this comment.
Result<(Array3<T>, Array3<T>), Error> would probably be more idiomatic (using the aliases where possible).
src/processing/sobel.rs
Outdated
| where | ||
| U: Data<Elem = T> + DataMut<Elem = T>, |
src/processing/sobel.rs
Outdated
| where | ||
| U: Data<Elem = T> + DataMut<Elem = T>, |
src/processing/sobel.rs
Outdated
| where | ||
| U: Data<Elem = T> + DataMut<Elem = T>, |
Also changed some types to be more idiomatic
|
I've resolved everything except that padding so I'm going to merge that and leave padding as a future challenge. Thanks for the review 👍 |
Rename the
Imagetype toImageBaseand have it takeT: Dataand useArrayBase<T, Ix3>as the underlying data type instead ofArray3<T>.Type alias
type Image = ImageBase<OwnedRepr<T>, _>created to replicate old behaviour. Examples and tests mostly unchanged so this shouldn't affect existing users (although they should be able to improve their code easily now)