Feat/autotune int ops #1136
Feat/autotune int ops #1136
Conversation
|
@louisfd Finally got the chance to start on #944. I'm going down the long route and trying to add support at large for |
Hi @agelas, thanks for tackling this issue! fn autotunables(&self) -> Vec<Box<dyn AutotuneOperation>> {
let random_bounds: (E, E) = ((-10.0).elem::<E>(), (10.0).elem::<E>());
let input = random_like_uniform(&self.input, random_bounds.0, random_bounds.1);because random_like_uniform only worked with floats even if E was an int. I'm not sure if you could just rewrite those lines to make it call your new int ops instead of the lower level random_like_uniform or if you'll have trouble determining if you need to call the float or int version. If that's the case then yes you can duplicate the AutotuneOperationSet, but it should keep the same key. In the end you will only have to change sum_dim / mean_dim implementations in fn sum_dim<const D: usize>(tensor: FloatTensor<Self, D>, dim: usize) -> FloatTensor<Self, D> {
#[cfg(feature = "autotune")]
{
reduce::sum_dim_autotune(tensor, dim)
}
#[cfg(not(feature = "autotune"))]
{
let output = init_reduce_output(&tensor, dim);
reduce::sum_dim(tensor, output, dim)
}
} |
| let random_in_range = (random_u32 % range) + low; | ||
|
|
||
| output[write_index] = random_in_range; | ||
| } |
There was a problem hiding this comment.
CI fails during cast_float. Can we skip this function as we don't want a float?
There was a problem hiding this comment.
@louisfd is this shader being invoked for the test that's failing? The problem on CI was a fusion test fusion::base::tests::aggregation::tests::test_should_mean_last_dim_int, that used to work at commit ae60d25 but then stopped working at dca5b26 for some reason. It works locally for me though.
Now it seems the CI is complaining about something else though.
There was a problem hiding this comment.
@louisfd Ok I'm back to failing on test_should_mean_last_dim_int() in the aggregation tests for burn-tensor. For both that and test_should_sum_last_dim_int(), shouldn't we be calling the int_mean_dim and int_sum_dim() though? Still not sure why cast_float() is causing issues though especially since I never use it, and if these tests use the base tensor functions, neither should even be calling anything from this PR.
#[test]
fn test_should_mean_last_dim_int() {
let tensor = TestTensorInt::from([[0, 1, 2], [3, 4, 5]]);
let data_actual = tensor.mean_dim(1).to_data();
assert_eq!(data_actual, Data::from([[1], [4]]));
}
#[test]
fn test_should_sum_last_dim_int() {
let tensor = TestTensorInt::from([[0, 1, 2], [3, 4, 5]]);
let data_actual = tensor.sum_dim(1).to_data();
assert_eq!(data_actual, Data::from([[3], [12]]));
}
|
The casting problem is quite obvious in fn cast_float(number: u32) -> {{ elem }} {
return 2.3283064365387e-10 * {{ elem }}(number);
}I think it should be : fn cast_elem(number: u32) -> {{ elem }} {
let tmp = 2.3283064365387e-10 * f32(number);
return {{ elem }}(tmp);
}It was assume that |
|
@nathanielsimard In @louisfd Also, for the test that's failing, is there a way to switch to use #[test]
fn test_should_mean_last_dim_int() {
let tensor = TestTensorInt::from([[0, 1, 2], [3, 4, 5]]);
let data_actual = tensor.mean_dim(1).to_data(); // <- use int_mean_dim(1) instead?
assert_eq!(data_actual, Data::from([[1], [4]]));
} |
Hi @agelas Sorry for the late reply. Regarding the cast functions, since it's getting a bit confusing I think we should be more explicit with the names: For your second question, maybe you're confusing tensor methods and backend operations? Because it's an int tensor the mean_dim method will call int_mean_dim underneath. impl<B: Backend> Numeric<B> for Int {
// ...
fn mean_dim<const D: usize>(tensor: Self::Primitive<D>, dim: usize) -> Self::Primitive<D> {
B::int_mean_dim(tensor, dim)
}
// ...
}For the CI error, it seems to be the modulo. Are you positive that both random_u32 and range are the same type? |
|
Hi @antimora @louisfd , sorry about the delay, life got in the way. @louisfd w/regards to the naming issue, I changed The CI is failing on codecov/path now at 63455c4 (the latest commit), I'm not sure what to do here since a lot of the code here gets used via a somewhat complicated mixture of macros and regular rust, |
|
@agelas, thank you for the update. Do not worry, the life is a priority. Regarding the coverage - it's not a hard requirement to hit 80%. We can override manually. |
Pull Request Template
Checklist
WIP
run-checks allscript has been executed.Related Issues/PRs
#944
Changes
Added int_random() to int tensor ops, new autotuning calls for sum_dim and mean_dim.
Testing
Unit tests for int_random(), sum_dim() and mean_dim() on WgpuTensors with IntElements .