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

n+1-1 in sum/prod #74

Closed
odashi opened this issue Nov 3, 2022 · 6 comments
Closed

n+1-1 in sum/prod #74

odashi opened this issue Nov 3, 2022 · 6 comments
Assignees
Labels
Milestone

Comments

@odashi
Copy link
Collaborator

odashi commented Nov 3, 2022

Currently the analyzer of sum and prod does not recognize the reducable addition and subtraction in the stop parameter of range:

It would be good if we could get $n$ rather than $n+1-1$ in most cases. This reduction would be safe because the operands of range must be integers, whose objects must be always associative and commutative in terms of operations $+$ and $-$.

@odashi odashi self-assigned this Nov 3, 2022
@odashi odashi added the feature label Nov 3, 2022
@odashi
Copy link
Collaborator Author

odashi commented Nov 3, 2022

Note that in general arithmetic is not associative even if all operands are numbers. For example:

So this treatment is available only when the function always takes integers.

@Casper-Guo
Copy link
Contributor

Implemented here:

elt_str = self.visit(node.elt)
target_str = self.visit(comp.target)
args_str = [self.visit(arg) for arg in comp.iter.args]
if len(args_str) == 1:
lower_str = "0"
upper_plus_1 = args_str[0]
else:
lower_str = args_str[0]
upper_plus_1 = args_str[1]
# Upper bound of range is exclusive. Try to numerically subtract it by 1.
try:
upper_plus_1_unwrapped = upper_plus_1[1:-1] # Remove { and }
upper_str = str(int(upper_plus_1_unwrapped) - 1)
except ValueError:
upper_str = "{" + upper_plus_1 + " - 1}"
# Used for: \sum_{target_str = lower_str}^{upper_str} elt_str
return elt_str, target_str, lower_str, upper_str

Current behavior is caused by not being able to convert n+1 to a numeric type.

The n+1 case can perhaps be treated as a special case since it is the most common but a more general solution might be preferred. Since reduction is only needed for n+x or n-x maybe we can explicitly check for these two cases.

@odashi
Copy link
Collaborator Author

odashi commented Nov 10, 2022

@Casper-Guo I think the code above is an obsolete permalink. In the current implementation, FunctionCodegen introduced an additional analyzer that obtains subtree of range:

range_info = analyzers.analyze_range(node.iter)

We can implement a specific process if we detected range_info.stop is BinOp and the operator is Add or Sub.

@Casper-Guo
Copy link
Contributor

We would like to work on this. Can you please assign me and @LakeBlair.

@odashi
Copy link
Collaborator Author

odashi commented Nov 10, 2022

@Casper-Guo Thanks for taking your effort! Assigned to you anyway since you are the only person who attended this discussion.

@odashi
Copy link
Collaborator Author

odashi commented Nov 28, 2022

Resolved by #115

@odashi odashi closed this as completed 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

No branches or pull requests

2 participants