Conversation
33409b3 to
5a198e8
Compare
vortex-array/src/scalar_fn/vtable.rs
Outdated
| Ok(args.to_vec()) | ||
| } | ||
|
|
||
| // TODO(connor): This needs a precondition for the number of args it has, or all implementations |
There was a problem hiding this comment.
There should already be a check using the arity of the function
There was a problem hiding this comment.
I tried backtracing through the code myself but I couldn't find where that is called
| fn is_fallible(&self, _options: &Self::Options) -> bool { | ||
| // Canonicalization of the storage array can fail. | ||
| true |
There was a problem hiding this comment.
This is not the meaning of this
There was a problem hiding this comment.
Its asks if the operation can fail liked checked_add (overflow). Can this happen here
There was a problem hiding this comment.
/// Returns whether this expression itself is fallible. Conservatively default to *true*.
///
/// An expression is runtime fallible is there is an input set that causes the expression to
/// panic or return an error, for example checked_add is fallible if there is overflow.
///
/// Note: this is only applicable to expressions that pass type-checking
/// [`ScalarFnVTable::return_dtype`].
In that case this doc comment needs to be more detailed since that was not clear. What is a better description here? How do we distinguish between an execution failure vs a logical failure when we get a VortexResult back regardless?
Merging this PR will not alter performance
Comparing Footnotes
|
615b78d to
2da0149
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
2da0149 to
01ea8b1
Compare
| // TODO(connor): This is just a placeholder for now. | ||
| type NativeValue<'a> = &'a ScalarValue; |
There was a problem hiding this comment.
It's mainly because we are blocked on #6717 and have to figure some things out on that first
There was a problem hiding this comment.
In reality we don't actually care about this now, but once we start using these extension types in the compressor then we do care because we would want a more efficient representation of a vector when (for example) encoded as a ConstantArray
| )?; | ||
|
|
||
| // Row 0: identical → 1.0, row 1: orthogonal → 0.0. | ||
| // Row 0: identical -> 1.0, row 1: orthogonal -> 0.0. |
| @@ -0,0 +1,236 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
just a personal taste thing - but these hyper specialized utils just crate fragmentation in the codebase, its more layers and they only exist in this corner
There was a problem hiding this comment.
Not really sure where else to put this though? We need it for both of the scalar fns in here (and will need it for more in the future)
There was a problem hiding this comment.
you can just inline them? they seem to be mostly very short and easy to quickly read through, even if it introduces some code duplication
There was a problem hiding this comment.
Hmmm I'm not sure I agree with this, especially since that would mean duplicating the FlatElements struct I introduced which feels wrong.
| /// For [`FixedShapeTensor`], computes `dot(a, b) / (||a|| * ||b||)` over the flat backing buffer of | ||
| /// each tensor. The shape and permutation do not affect the result because cosine similarity only | ||
| /// depends on the element values, not their logical arrangement. | ||
| /// Computes `dot(a, b) / (||a|| * ||b||)` over the flat backing buffer of each tensor or vector. |
There was a problem hiding this comment.
Not part of this PR, but I think we also want to do expr(array, scalar) for this sort of thing
There was a problem hiding this comment.
yeah that is also part of why we want #6717
AdamGS
left a comment
There was a problem hiding this comment.
This seems very reasonable to me, some pieces depend on future work, like Variant we probably want a standard way to express these sort of stability/maturity guarantees
01ea8b1 to
30c4258
Compare
|
|
||
| fn id(&self) -> ExtId { | ||
| ExtId::new_ref("vortex.fixed_shape_tensor") | ||
| ExtId::new_ref("vortex.tensor.fixed_shape_tensor") |
There was a problem hiding this comment.
Are we certain no one has written a vortex.fixed_shape_tensor?
There was a problem hiding this comment.
We're pretty sure, this is a very recent addition
| .map(|i| l2_norm_row(flat.row::<T>(i))) | ||
| .collect(); | ||
|
|
||
| Ok(result.into_array()) |
There was a problem hiding this comment.
Do we intend to eventually replace this with a call to BLAS? It seems like extract_flat_elements produces a BLAS compatible matrix.
There was a problem hiding this comment.
Basic Linear Algebra Subprograms. It's the way to do fast linear algebra. There's also LAPACK which mostly deals with matrix transformations e.g. QR factorization.
There's many implementations of the BLAS interface. Perhaps the most interesting is GotoBLAS which this guy hand wrote in assembly. If you're on Intel processors you really want to link against the Intel Matrix Kernel Library (MKL). There's also OpenBLAS which is arch independent and cuBLAS which is for GPUs. Generally, one expects these libraries to be installed on the machine and you dynamically link against them.
There was a problem hiding this comment.
is there a standard rust crate that does this? If we can pull that in and it operates over flat elements then that is a trivial thing to add. I looked online and found a few things but am not familiar with this space
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Summary
Tracking Issue: #6865
Adds a
Vectorextension type and a newL2Normexpression.Additionally adds a
AnyTensortype that can be matched on for any kind of tensor we want.Right now the code assumes that everything is built on top of
FixedSizeList, but in the future that might change.Additionally make some touchups to the
vortex-tensorcrate in general.API Changes
The new
VectorandL2Normtypes.Testing
Some basic tests.