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

Addition / Subtraction Simplification for the stop parameter in sum / prod #115

Merged
merged 7 commits into from
Nov 17, 2022

Conversation

Casper-Guo
Copy link
Contributor

Overview

This PR addresses issue #74.

Details

Implementation

First check the stop parameter of range_info is ast.BinOp and represents an addition or subtraction. If so, the minus one is applied to BinOp.right and a new ast.BinOp node is created. Then the existing visit function is leveraged to create the needed Latex.

Note the special cases + 1 - 1 and - (-1) - 1 for which only BinOp.left is kept.

Added test cases to regression_test.py. Please let me know if these are more appropriate elsewhere (e.g. function_codegen_test.py)

Caveats

  • n+1-1 can be reduced to n but 1+n-1 cannot. This is because range(1+n) is unlikely and unusual. Support for this can be easily added if desired
  • chained additions and subtractions cannot be fully reduced. n + 3+4-1 will become n+3+3 not n+6. This is in line with how other chained BinOps are currently handled and may be addressed in a future issue

Questions

The implementation is slightly verbose and perhaps should be written as a separate function. Might other future PRs need this similar functionality?

References

Issue #74

@Casper-Guo Casper-Guo requested a review from odashi as a code owner November 16, 2022 05:40
@google-cla
Copy link

google-cla bot commented Nov 16, 2022

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.

@Casper-Guo Casper-Guo changed the title Issue 74 dev Addition / Subtraction Simplification for the stop parameter in sum / prod Nov 16, 2022
Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

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

The logic looks good, thanks!
Please add appropriate unit tests into function_codegen_test.py as well.

@Casper-Guo Casper-Guo requested a review from odashi November 16, 2022 23:43
@odashi
Copy link
Collaborator

odashi commented Nov 17, 2022

@Casper-Guo Could you resolve the conflicts introduced by recent changes on main?

@odashi
Copy link
Collaborator

odashi commented Nov 17, 2022

@Casper-Guo It is not easy to select only your stuff. Try git merge main to remove duplicated changes.

@Casper-Guo
Copy link
Contributor Author

Casper-Guo commented Nov 17, 2022

@Casper-Guo It is not easy to select only your stuff. Try git merge main to remove duplicated changes.

I rebased on main before pushing the new changes which should conceal all the new commits... Not sure why it didn't turn out that way. I will try to figure out a way to tidy it up.

Edit: Should be all set for your review now. Sorry for the confusion

@odashi
Copy link
Collaborator

odashi commented Nov 17, 2022

Looks great, thanks!

@odashi odashi merged commit e7a74f7 into google:main Nov 17, 2022
@odashi odashi added the feature label Nov 17, 2022
@odashi odashi added this to the v0.3 milestone Nov 17, 2022
@Casper-Guo Casper-Guo deleted the issue-74-dev branch November 17, 2022 15:31
@odashi odashi mentioned this pull request Nov 28, 2022
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 this pull request may close these issues.

2 participants