-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Plan for dimension types #519
Comments
It looks cleaner, which is always good API-wise, but I have to admit that the whole dimension-shape situation is a little big foggy to me (especially since reading #367, I feel there is a lot I am missing there between the lines). To get a proper understanding of what this implies could you frame it with respect to the other dimension-related traits? |
isize vs usize genericity is a low level problem. It would be nice to solve. Before those come other problems I think: possible division of a trait for indices vs a trait dimensions (current solution uses the same for both which is pragmatic but limiting), and like you point out, making the dimension-related traits more obvious and hopefully fewer in number. |
I was rereading #367 and I think, even though there might some technical difficulties that will have to be ironed out, that what @termoshtt was proposing makes intuitive sense from an API point of view. I would actually push it one step further, by further decomposing what we know call
A From a user point of view, I think this would result more "intuitive". |
Yeah, I think it makes sense to separate the type that implements use num_traits::Zero;
use smallvec::SmallVec;
// Replaces the "number of dimensions" portion of `Dimension`
pub trait Dimensionality {
type Shape: DimRepr<Elem = usize>;
type Strides: DimRepr<Elem = isize>;
const NDIM: Option<usize>;
}
pub struct D1;
pub struct D2;
pub struct D3;
pub struct D4;
pub struct D5;
pub struct D6;
pub struct Ddyn;
impl Dimensionality for D2 {
type Shape = [usize; 2];
type Strides = [isize; 2];
const NDIM: Option<usize> = Some(2);
}
impl Dimensionality for Ddyn {
type Shape = DynRepr<usize>;
type Strides = DynRepr<isize>;
const NDIM: Option<usize> = None;
}
// Replaces the "representation" portion of `Dimension`
pub trait DimRepr {
type Elem: Clone;
type Dim: Dimensionality;
fn ndim(&self) -> usize;
fn from_slice(slice: &[Self::Elem]) -> Self;
fn as_slice(&self) -> &[Self::Elem];
fn as_mut_slice(&mut self) -> &mut [Self::Elem];
}
impl<T: Clone> DimRepr for [T; 2] {
type Elem = T;
type Dim = D2;
fn ndim(&self) -> usize { 2 }
fn from_slice(slice: &[T]) -> Self {
assert_eq!(slice.len(), 2);
[slice[0].clone(), slice[1].clone()]
}
fn as_slice(&self) -> &[T] { &self[..] }
fn as_mut_slice(&mut self) -> &mut [T] { &mut self[..] }
}
pub struct DynRepr<T>(SmallVec<[T; 4]>);
impl<T: Clone> DimRepr for DynRepr<T> {
type Elem = T;
type Dim = Ddyn;
fn ndim(&self) -> usize { self.0.len() }
fn from_slice(slice: &[T]) -> Self { DynRepr(slice.into()) }
fn as_slice(&self) -> &[T] { &self.0 }
fn as_mut_slice(&mut self) -> &mut [T] { &mut self.0 }
}
pub struct ArrayBase<S, D>
where
S: Data,
D: Dimensionality,
{
data: S,
ptr: *mut S::Elem,
shape: D::Shape,
strides: D::Strides,
}
impl<A, S, D> ArrayBase<S, D>
where
S: Data<Elem = A>,
D: Dimensionality,
{
// Equivalent to old zeros method. The only real difference is that
// `ShapeBuilder` has been renamed to `LayoutBuilder`.
pub fn zeros<L>(layout: L) -> Self
where
A: Clone + Zero,
S: DataOwned,
L: LayoutBuilder<Dim = D>,
{
unimplemented!()
}
/// Errors if [...] any of the strides are negative.
pub fn from_shape_vec<L>(layout: L, v: Vec<A>) -> Result<Self, ShapeError>
where
L: Into<StridedLayout<D>>,
{
unimplemented!()
}
pub fn into_shape<E>(self, shape: E) -> Result<ArrayBase<S, E::Dim>, ShapeError>
where
E: IntoShape,
{
unimplemented!()
}
pub fn shape(&self) -> &D::Shape {
&self.shape
}
pub fn strides(&self) -> &D::Strides {
&self.strides
}
}
// Replaces the old `IntoDimension` trait
pub trait IntoShape {
type Dim: Dimensionality;
fn into_shape(self) -> <Self::Dim as Dimensionality>::Shape;
}
pub trait IntoStrides {
type Dim: Dimensionality;
fn into_strides(self) -> <Self::Dim as Dimensionality>::Strides;
}
// Equivalent to the old `Shape` struct
pub struct ContigLayout<D: Dimensionality> {
shape: D::Shape,
is_c: bool,
}
// Equivalent to the old `StrideShape` struct
pub struct StridedLayout<D: Dimensionality> {
shape: D::Shape,
strides: D::Strides,
is_custom: bool,
}
// Nearly equivalent to the old `ShapeBuilder` trait
pub trait LayoutBuilder {
type Dim: Dimensionality;
fn into_contig_layout(self) -> ContigLayout<Self::Dim>;
fn f(self) -> ContigLayout<Self::Dim>;
fn set_f(self, is_f: bool) -> ContigLayout<Self::Dim>;
fn strides<St>(self, strides: St) -> StridedLayout<Self::Dim>
where
St: IntoStrides<Dim = Self::Dim>;
} This is similar to the existing implementation; the primary changes are:
Everything else about the existing implementation stays pretty much the same, other than renaming some things. I've chosen to expose the representation of fixed-dimension shape/strides as fixed-length arrays, but that's not strictly necessary. The remaining things I dislike about this are:
|
I was going to say first that in general:
I like your new approach jturner. It's good but doesn't seem complete, unsure if it solves enough problems. I'll ask about a concrete problem in Array::zeros fn zeros<Sh>(new_shape: Sh) where Sh: ShapeBuilder<Dim=D> This would be better with a bound like For array constructors we have three kinds of inputs:
For natural reasons we only allow (1) and (2) for most constructors, and (3) for some advanced constructors, and this is the reason we use the type system to only accept (3) where we can support it. Current bounds accomplish this, but the traits and trait names don't make it very clear. For me, "Layout" means something like C/F memory layout, it doesn't mean the lengths of each axis, so I'm unsure about the name. |
I like @jturner314's design - wouldn't it be better though to use two separate traits for Shapes and Strides? (even if they might offer exactly the same methods initially) |
Having worked with numpy and also some Eigen3 in C++ I was wondering if there are plans to support having the size (of some dimensions) as part of the type. Something along with Edit: AFAI this is not (yet) possible with rust, as it would either require variadic const generics or const generics to support more than |
I've been thinking about the dimension types for a while. Two things I don't like about the current implementation are that:
usize
and have to be converted toisize
every time they're used. This is error-prone and confusing.What I'd like to do is something like the following:
Once Rust has generic associated types, we can simplify this to:
I'd also add various type-level arithmetic operations on the dimension types, which are necessary for things like co-broadcasting (
trait PartialOrdDim
) andfold_axes
(trait SubDim
).We can also add
Shape<T>
,Strides<T>
, andIndex<T>
thin wrapper types.This approach would resolve things like #489 and this comment on #367.
Thoughts?
The text was updated successfully, but these errors were encountered: