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

Add MatMul operator to patchedast #497

Merged
merged 5 commits into from Jul 18, 2022
Merged

Add MatMul operator to patchedast #497

merged 5 commits into from Jul 18, 2022

Conversation

lieryan
Copy link
Member

@lieryan lieryan commented Jul 16, 2022

Description

Fix issue with parsing code with MatMul operator.

Fixes #496

Checklist (delete if not relevant):

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md

@lieryan lieryan added this to the 1.3.0 milestone Jul 16, 2022
@lieryan
Copy link
Member Author

lieryan commented Jul 16, 2022

Hi @eivindjahren, I wasn't able to replicate the issue, but this PR should fix this issue. Let me know if you would be able to test this branch, or when 1.3.0 is released.

@eivindjahren
Copy link

eivindjahren commented Jul 16, 2022

Unfortunately it doesn't seem to fix the problem as the name of the operator is MatMult and not MatMul.

The following regression test (when added to ropetest/refactor/patchedasttest.py reproduces the bug:

    def test_matmul_node(self):
        source = (
            "import numpy as np\n"
            "a=np.array([[1,2]])\n"
            "b=np.array([[1],[2]])\n"
            "a @ b\n"
        )
        ast_frag = patchedast.get_patched_ast(source, True)
        checker = _ResultChecker(self, ast_frag)
        checker.check_children("BinOp", ["Name", " ", "@", " ", "Name"])

I think you probably can skip the import and use of numpy by just using lists. As I understand it, that would just result in a runtime error where it is discovered that a and b do not implement __matmul__. However, adding the numpy import perhaps is more clear when it comes to how this is used.

@@ -277,6 +277,7 @@ def _child_nodes(self, nodes, separator):
"Div": "/",
"Mod": "%",
"Pow": "**",
"MatMul": "@",

Choose a reason for hiding this comment

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

Should be "MatMult": "@",

Choose a reason for hiding this comment

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

Fixed now

@eivindjahren
Copy link

eivindjahren commented Jul 17, 2022

The bug is fixed now!🎉 I prefer to add a regression test with every bug-fix. I find its a good way to grow test coverage. The test I pasted above should suffice. But its just my preference and you call the shots 🔫 , so feel free to just ignore that :P

@lieryan lieryan enabled auto-merge July 18, 2022 06:06
@eivindjahren
Copy link

Nice 👍

@lieryan lieryan merged commit 51b1bf8 into master Jul 18, 2022
@lieryan lieryan deleted the lieryan-496-matmul-op branch July 18, 2022 06:29
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.

Rename fails with missing KeyError: MatMul in patchedast
2 participants