Skip to content
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

Replace Vec<Vec<T>> with a native Matrix representation #274

Open
hackaugusto opened this issue Apr 26, 2023 · 5 comments
Open

Replace Vec<Vec<T>> with a native Matrix representation #274

hackaugusto opened this issue Apr 26, 2023 · 5 comments

Comments

@hackaugusto
Copy link
Contributor

hackaugusto commented Apr 26, 2023

Matrixes are currently represented with:

Matrix(Vec<Vec<u64>>),

The representation above has two issues:

  • there is no guarantee the matrix size is correctly, i.e. the following can be created [[1], [2,3]]
  • there is one allocation per row (probably not critical here, but always nice to reduce unnecessary allocations)

One sketch proposal is:

struct Matrix<T, const N: usize, const M: usize> {
    data: Vec<T>,
}

impl<T, const N: usize, const M: usize> Matrix<T, N, M> {
    fn new() -> Matrix<T, N, M> {
        let data = Vec::with_capacity(N * M);
        Matrix { data }
    }
}

Note: The above would be statically typed, another approach is to have a dynamic view of the data, like with pandas data frames.

@jan-ferdinand
Copy link

A crate worth considering is the imho excellent ndarray.

@hackaugusto
Copy link
Contributor Author

hackaugusto commented Apr 26, 2023

Note: The validation of the matrix is currently done here:

// check the number of elements in each row are same for a matrix
if let ConstantValueExpr::Matrix(matrix) = &constant_type {
let row_len = matrix[0].len();
if matrix.iter().skip(1).any(|row| row.len() != row_len) {
return Err(SemanticError::invalid_matrix_constant(&name));
}
}

Is the idea to air-script-core with parsed but not-yet-validated data?

Edit: I think it makes sense to have a separate type that performs validation in the constructor, it doesn't have to replace the structure I linked in the original issue, but having a type with these properties validated makes easier to audit (since one doesn't have to find were / if the validation is done).

@tohrnii
Copy link
Contributor

tohrnii commented Apr 26, 2023

Is the idea to air-script-core with parsed but not-yet-validated data?

We tried to keep validation out of the grammar in many cases in general to avoid making the grammar unnecessarily complex, but I agree with you that in this case it probably makes sense to do the validation in the constructor at the parser level.

@hackaugusto
Copy link
Contributor Author

We tried to keep validation out of the grammar in many cases in general to avoid making the grammar unnecessarily complex

Thanks for the explanation, makes sense! How does the core crate fit into that?

@tohrnii
Copy link
Contributor

tohrnii commented Apr 27, 2023

How does the core crate fit into that?

The idea behind core crate was just to have commonly used structs/consts to be defined there (similar to miden-vm-core) so that it can be used as a dependency in other crates. For example, codegen doesn't depend on parser crate but imports some structs from core crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants