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

Added remaining Numpy NDArray single function expressions #183

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

aritrakar
Copy link
Contributor

Overview

This pull request expands support for several NumPy linear algebra functions involving a single operand (as mentioned in #149), including numpy.linalg.{matrix_power, qr, svd, det, matrix_rank, inv, pinv}. It introduces support for applying these functions to both variables and NDArrays, improving the versatility of the codebase.

Details

Key changes in this PR:

  • Added new functions and modified existing code in expression_codegen.py to support the specified Numpy linear algebra functions.
  • Included comprehensive test cases in expression_codegen_test.py to validate the correctness of the new implementations.
  • Followed a coding style consistent with the NumPy transpose implementation.

References

#149

Blocked by

N/A

…s: numpy.linalg.{matrix_power, qr, svd, det, matrix_rank, inv, pinv}.
@aritrakar aritrakar requested a review from odashi as a code owner October 17, 2023 02:43
@google-cla
Copy link

google-cla bot commented Oct 17, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@odashi
Copy link
Collaborator

odashi commented Nov 12, 2023

Hey @aritrakar, thanks for proposing the change! Please sign the CLA to proceed the PR.

@aritrakar
Copy link
Contributor Author

Hey @aritrakar, thanks for proposing the change! Please sign the CLA to proceed the PR.

Hey @odashi , nice to hear from you. I'd signed the CLA after I saw the error. Do I need to sign a new one?

@odashi
Copy link
Collaborator

odashi commented Nov 13, 2023

@aritrakar Ah I see, maybe I'd overlooked something.

@odashi
Copy link
Collaborator

odashi commented Nov 14, 2023

@aritrakar It looks some CIs are still failing. You can check them locally by running checks.sh with installing necessary libraries.

@aritrakar
Copy link
Contributor Author

aritrakar commented Nov 15, 2023

Hey @odashi , sorry about that. I fixed the issues related to line lengths. I checked the CI tests locally this time and there seem to be some issues in files that I haven't touched. Do you know what's going on?

For reference, this is what I see when I run checks.sh:

image

@odashi
Copy link
Collaborator

odashi commented Nov 15, 2023

@aritrakar It looks you are using Python older than 3.10. the Match syntax was introduced by that version and any versions older than 3.10 can't process the syntax.

@aritrakar
Copy link
Contributor Author

Right. I've upgraded to use Python 3.11.6 now and the errors related to Match have disappeared. However, the errors related to flake8 and mypy seem to persist and looks like they're common across PRs.

For reference:

image

@odashi
Copy link
Collaborator

odashi commented Nov 15, 2023

@aritrakar Yeah They are common as I mentioned in another PR:

#190 (comment)

I will handle this error on this PR as well.


func_arg = node.args[0]
if isinstance(func_arg, ast.Name):
return rf"\det \left( \mathbf{{{func_arg.id}}} \right)"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks some generation rules don't follow the convention of the library: \mathopen{}\left* and \mathclose{}\right*

Comment on lines 315 to 338
def _generate_qr_and_svd(self, node: ast.Call) -> str | None:
"""Generates LaTeX for numpy.linalg.qr and numpy.linalg.svd.
Args:
node: ast.Call node containing the appropriate method invocation.
Returns:
Generated LaTeX, or None if the node has unsupported syntax.
Raises:
LatexifyError: Unsupported argument type given.
"""
name = ast_utils.extract_function_name_or_none(node)
assert name == "QR" or name == "SVD"

if len(node.args) != 1:
return None

func_arg = node.args[0]
if isinstance(func_arg, ast.Name):
func_arg_str = rf"\mathbf{{{func_arg.id}}}"
return rf"\mathrm{{{name.upper()}}} \left( {func_arg_str} \right)"

elif isinstance(func_arg, ast.List):
matrix_str = self._generate_matrix(node)
return rf"\mathrm{{{name.upper()}}} \left( {matrix_str} \right)"
return None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can be removed. The current implementation converts QR(A) to $\mathrm{QR}(A)$ and SVD(A) to $\mathrm{SVD}(A)$

return rf"\mathrm{{{name.upper()}}} \left( {matrix_str} \right)"
return None

def _generate_inverses(self, node: ast.Call) -> str | None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks there is no reason to combine inv and pinv into the same function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, would you rather I separate them into two different functions?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that would be reasonable I think.

@odashi
Copy link
Collaborator

odashi commented Nov 15, 2023

@aritrakar Merging main would resolve the CI issues (#191)

@odashi
Copy link
Collaborator

odashi commented Nov 17, 2023

@aritrakar Thanks for a bunch of effort!

@odashi odashi merged commit 1cfb8e7 into google:main Nov 17, 2023
10 checks passed
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

Successfully merging this pull request may close these issues.

2 participants