Repository navigation
Conversation
ValerianRey
left a comment
ValerianRey
left a comment
There was a problem hiding this comment.
I think this leads to some issues, the main one being that we would want to have the different parts of a MultiDomain be able to be of different type if we want this to be fully general. I think we should rather aim for that. But also, I just realized that all of this may not be necessary even for einsums, because we can right-compose each operand with the InvProjection transform before stacking them, so that they're all in the (same, HRect) latent domain before stacking. So maybe it's best if we wait for this to become really necessary (if it ever becomes necessary) before working more on it.
| /// Number of coordinates the indices of this domain have, or `None` when | ||
| /// they don't all have the same number, as in a multi-domain whose parts | ||
| /// differ in rank. | ||
| fn ndim(&self) -> Option<usize>; |
There was a problem hiding this comment.
I'm wondering if ndim should be a property of HRect since not all domains necessarily have a ndim. The only thing bother me is that some domains are not hrect but still have ndim, e.g. a tensor of 4 dimensions but whose last dimension can have various lengths based on the values of the 3 first dimensions (this is the former JaggedDomain).
I'm not entirely sure what we could do, but if we could simply move the ndim function to HRect instead of Domain it would be nice IMO.
|
|
||
| /// A shared copy of this domain. A tensor only lends out its domain, so this | ||
| /// is how a domain built over other tensors' domains holds on to them. | ||
| fn to_rc(&self) -> Rc<dyn Domain>; |
There was a problem hiding this comment.
Is it standard to have a to_rc function? I don't think we had any before.
| fn to_rc(&self) -> Rc<dyn Domain>; | ||
| } | ||
|
|
||
| impl Domain for Rc<dyn Domain> { |
There was a problem hiding this comment.
Is this some kind of trick so that we can use a ref-counted dyn domain as if it was a domain? Not sure to understand
| parts.iter().all(|part| part.ndim() == ndim), | ||
| "the parts of a jagged domain must all have the same number of coordinates" | ||
| ); | ||
| impl<D: Domain> MultiDomain<D> { |
There was a problem hiding this comment.
What if we want to have a MultiDomain consisting of domains of different types? I think this only covers domains of the same type, right?
| impl<D: Domain + Clone + 'static> Domain for MultiDomain<D> { | ||
| fn ndim(&self) -> Option<usize> { | ||
| let rank = self.parts[0].ndim()?; | ||
| self.parts | ||
| .iter() | ||
| .all(|part| part.ndim() == Some(rank)) | ||
| .then_some(1 + rank) |
There was a problem hiding this comment.
Can't we just say that MultiDomain doesn't have a ndim property? Or is it needed sometimes?
| /// transpose. | ||
| pub fn transpose<T: BoxedTensor + 'static>(u: Rc<T>) -> RightComposition<Permutation> { | ||
| let ndim = u.domain().ndim(); | ||
| let ndim = u.hrect().shape().len(); |
There was a problem hiding this comment.
well, technically, u is a BoxedTensor, so its domain is a HRect which should always have a ndim, no? There are several occurrences of this.
| let codomain = JaggedDomain::new( | ||
| tensors | ||
| .iter() | ||
| .map(|tensor| tensor.hrect().clone()) | ||
| .collect(), | ||
| ); |
There was a problem hiding this comment.
technically, we could construct the underlying first, type it as JaggedTensor (which I think it still is), and then get its jagged_domain for codomain, no?
MultiDomain<D>: parts can be any domain, of any rank.JaggedDomainis nowMultiDomain<HRect>MultiTensorreplacesJaggedTensorand holds any tensorsDomain::ndimreturnsOption<usize>(Nonewhen parts differ in rank), and domains getto_rc()to make a shared copyStackmapping: stacks mappings of one domain, so tensors of different ranks can be read as one boxed tensor (tested with a matrix and a vector broadcast along its rows)Whereis generic over its domain;where_is unchangedCloses #5