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

NDArray support #78

Closed
odashi opened this issue Nov 4, 2022 · 7 comments · Fixed by #143
Closed

NDArray support #78

odashi opened this issue Nov 4, 2022 · 7 comments · Fixed by #143
Assignees
Labels
Milestone

Comments

@odashi
Copy link
Collaborator

odashi commented Nov 4, 2022

Sometimes users want to support NDArrays in this library, e.g., np.array([[a, b], [c, d]]) to

$$\left( \begin{array}{cc}a&b \\ c&d\end{array} \right)$$

This is definitely useful, but often it is not possible to compile:

  • Large arrays.
  • Arrays with more than 2 dimensions.

I think we could start implementing it with reasonable restrictions (e.g., only NDArrays with <= 2 dimensions with small lengths), but it would be also good to keep discussion.

Refs:

@odashi odashi self-assigned this Nov 4, 2022
@odashi odashi added the feature label Nov 4, 2022
@odashi odashi added this to the v0.3 milestone Nov 4, 2022
@Casper-Guo
Copy link
Contributor

For 2D arrays, limiting the maximum size to 10 x 10 is perhaps a reasonable starting point. Larger arrays, especially if they are dense, are not great as visual aids even if they are rendered into LaTex.

@moringspeaker
Copy link

For 2D arrays, limiting the maximum size to 10 x 10 is perhaps a reasonable starting point. Larger arrays, especially if they are dense, are not great as visual aids even if they are rendered into LaTex.

I think it's a great idea to constraint the maximum size of matrices. I won't use matrix larger than 10 * 10 in Latex.

@odashi
Copy link
Collaborator Author

odashi commented Nov 10, 2022

@Casper-Guo Yes I think it is reasonable. maybe we restrict to show only $(R, 1)$, $(1, C)$ and $(R, C)$ arrays with size $R \le R_{max}$ and $C \le C{max}$, where $R_{max}$ and $C_{max}$ are configs defaulted to some number (10 for example)

@Casper-Guo
Copy link
Contributor

Casper-Guo commented Nov 10, 2022

What should the configuration interface look like? I feel this should be file level rather than function level.

cc: @LakeBlair

@odashi
Copy link
Collaborator Author

odashi commented Nov 10, 2022

@Casper-Guo We can start with hard-coding the constraint into the code until we decided the structure of config files.

@kshxtij
Copy link
Contributor

kshxtij commented Nov 16, 2022

I was able to implement a basic version of this within src/latexify/function_codegen.py by editing visit_Call

        if func_str == "ndarray":
            # construct matrix
            matrix_str = r"\begin{bmatrix} "
            # iterate over rows
            for row in node.args[0].elts:
                for col in row.elts:
                    matrix_str += self.visit(col) + r" & "
                matrix_str = matrix_str[:-2] + r" \\ "
            matrix_str = matrix_str[:-3] + r"\end{bmatrix}"
            return matrix_str

Screenshot 2022-11-16 at 16 35 54

However, this isn't very extendable especially if we want to support functions like np.eye or np.ones or np.zeroes, so do you have any suggestions on how i can implement this better?

We can also potentially support multiple types of matrix formatting (square brackets vs round brackets vs curly brackets) via a config option?

@Casper-Guo
Copy link
Contributor

Casper-Guo commented Nov 16, 2022

np.eye, np.ones and other matrix functions will probably be dealt with separately. I think the biggest issue with your current implementation is that it doesn’t generalize to 1D arrays / vectors

If anything, the matrix processing should probably be a separate function instead of resting in, say a visit_x. That will make implementing support for np.inv and np.transpose etc. more straightforward

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

Successfully merging a pull request may close this issue.

4 participants