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

Support integer division with // operator #9

Closed
rmshaffer opened this issue May 8, 2024 · 12 comments · Fixed by #29
Closed

Support integer division with // operator #9

rmshaffer opened this issue May 8, 2024 · 12 comments · Fixed by #29
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@rmshaffer
Copy link
Contributor

Users should be able to perform integer division on variables in the program. For example:

a = aq.IntVar(7)
b = a // 2

In fixing this we must ensure that integer division operations are generated according to the OpenQASM spec.
For example, see:

And any Python floating-point division operations should also be converted into appropriate floating-point operations in the OpenQASM.

@rmshaffer rmshaffer converted this from a draft issue May 8, 2024
@rmshaffer rmshaffer added enhancement New feature or request good first issue Good for newcomers labels May 8, 2024
@daredevil3435
Copy link
Contributor

Hi, I am complete begineer in Quantum computing but I do have programming experience. Could you guide me through this? I would love to take up this issue.

@rmshaffer
Copy link
Contributor Author

@daredevil3435 - thanks for your interest! This issue actually requires very little awareness of quantum computing, so it may be a good place to start for you. This issue will require translating the Python // operator into the appropriate operations in the oqpy.Program, if the arguments to the // are also variables in the program.

A good place to start is to look at how we currently do this for comparison operators <, <=, >, >=.

First, there is a converter defined here in the converters module:
https://github.com/amazon-braket/autoqasm/blob/main/src/autoqasm/converters/comparisons.py
which converts these to operator function calls (inside visit_Compare).

Then, the operator functions (lt_, lteq_, etc.) themselves are defined here in the operators module:
https://github.com/amazon-braket/autoqasm/blob/main/src/autoqasm/operators/comparisons.py
which either evaluates the Python operator or adds a node to the oqpy program, as appropriate.

Finally, you will need to call your new converter from the transpiler here, to ensure that it gets invoked when transpiling the AutoQASM program:

# autograph converters
node = functions.transform(node, ctx)
node = directives.transform(node, ctx)
node = break_statements.transform(node, ctx)
node = asserts.transform(node, ctx)
# Note: sequencing continue canonicalization before for loop one avoids
# dealing with the extra loop increment operation that the for
# canonicalization creates.
node = continue_statements.transform(node, ctx)
node = return_statements.transform(node, ctx)
node = assignments.transform(node, ctx)
node = lists.transform(node, ctx)
node = slices.transform(node, ctx)
node = call_trees.transform(node, ctx)
node = control_flow.transform(node, ctx)
node = conditional_expressions.transform(node, ctx)
node = comparisons.transform(node, ctx)
node = logical_expressions.transform(node, ctx)
node = variables.transform(node, ctx)

Feel free to give that a try, and let me know if you have any specific questions!

@daredevil3435
Copy link
Contributor

daredevil3435 commented Jun 6, 2024

so basically I need to

  1. Add new fd_ and _aq_fd functions in operator/comparioson.py (fd -> floordivision)
  2. Add _aq_fd in operators in converters/comparison.py
  3. i quite did not understand third step you mentioned.
    I am writing this in order to see whether I am correctly interpreting your message. Plus the return type for // will be int and IntegerLiteral?

@rmshaffer
Copy link
Contributor Author

@daredevil3435 yes, I think that's basically it! The third step is just adding one line of code - a call to the transform of your new converter that you created in the first step.

@daredevil3435
Copy link
Contributor

I have added new function, converter in it. Started doing unit tests but it gave me an error. Should I open a PR for you to review my changes if they're correct or not?

@abidart
Copy link
Contributor

abidart commented Jun 11, 2024

Hello @daredevil3435 and @rmshaffer, I just opened a PR ( #29 ) to address this issue, building on the work @daredevil3435 had already done but changing a few key things. There is not much time left for UnitaryHack so maybe we can work together to close the issue 🤝 and split the bounty 💰? Otherwise, I am happy to step aside since the issue had been claimed already.

I was also wondering what the behavior should be when the user uses integer division between a FloatVar and an IntVar. For example, 3.1 // 2 in Python results in 1.0. Would this be the equivalent of casting the IntVar to a FloatVar, using "normal" division, casting the result of the division to IntVar to trim the decimal values, and then cast to FloatVar again? Or should we assume the user expects an IntVar as a result? Any thoughts?

@rmshaffer
Copy link
Contributor Author

rmshaffer commented Jun 11, 2024

I was also wondering what the behavior should be when the user uses integer division between a FloatVar and an IntVar. For example, 3.1 // 2 in Python results in 1.0. Would this be the equivalent of casting the IntVar to a FloatVar, using "normal" division, casting the result of the division to IntVar to trim the decimal values, and then cast to FloatVar again? Or should we assume the user expects an IntVar as a result? Any thoughts?

Thanks for taking a look at this @abidart! That's a great question - I think having integer division result in an int, as you've done in your PR, is logical behavior. But I guess for the best consistency with Python, as you point out, it should be a float. I'm not sure there would be much difference in program behavior, since the numeric value will be the same. I could be convinced either way.

@daredevil3435
Copy link
Contributor

Hello @daredevil3435 and @rmshaffer, I just opened a PR ( #29 ) to address this issue, building on the work @daredevil3435 had already done but changing a few key things. There is not much time left for UnitaryHack so maybe we can work together to close the issue 🤝 and split the bounty 💰? Otherwise, I am happy to step aside since the issue had been claimed already.

I was also wondering what the behavior should be when the user uses integer division between a FloatVar and an IntVar. For example, 3.1 // 2 in Python results in 1.0. Would this be the equivalent of casting the IntVar to a FloatVar, using "normal" division, casting the result of the division to IntVar to trim the decimal values, and then cast to FloatVar again? Or should we assume the user expects an IntVar as a result? Any thoughts?

Hi. I actually did a lot of hard work on this issue as basically it is my first ever bug bounty. I have already done major part of the work. So if you want to carry it forward let's split the bounty.

@abidart
Copy link
Contributor

abidart commented Jun 11, 2024

@daredevil3435 sounds great, thank you very much! I will try my best to wrap it up ASAP (hopefully we will make it on time).

Also, feel free to DM me on the server (my username is abid). Let me know if you have any questions or suggestions for the PR. I've been working on the codebase too for the last couple of days—also learning a lot—and we can definitely chat about it!

@rmshaffer
Copy link
Contributor Author

@daredevil3435 @abidart thank you both! I have no doubt we'll get this wrapped up in time. I'll try to keep reviewing your changes promptly.

@abidart
Copy link
Contributor

abidart commented Jun 11, 2024

I was also wondering what the behavior should be when the user uses integer division between a FloatVar and an IntVar. For example, 3.1 // 2 in Python results in 1.0. Would this be the equivalent of casting the IntVar to a FloatVar, using "normal" division, casting the result of the division to IntVar to trim the decimal values, and then cast to FloatVar again? Or should we assume the user expects an IntVar as a result? Any thoughts?

Thanks for taking a look at this @abidart! That's a great question - I think having integer division result in an int, as you've done in your PR, is logical behavior. But I guess for the best consistency with Python, as you point out, it should be a float. I'm not sure there would be much difference in program behavior, since the numeric value will be the same. I could be convinced either way.

Given that the Python docs state:

Also referred to as integer division. For operands of type int, the result has type int. For operands of type float, the result has type float. In general, the result is a whole integer, though the result’s type is not necessarily int. The result is always rounded towards minus infinity: 1//2 is 0, (-1)//2 is -1, 1//(-2) is -1, and (-1)//(-2) is 0.

I am leaning towards returning a FloatVar whenever either of the operands is a FloatVar. Imitating the Python behavior is probably the best way to prevent issues stemming from integer division.

I made the corresponding changes to the PR, but I am happy to backtrack if you @rmshaffer think it would be best to do otherwise.

@rmshaffer
Copy link
Contributor Author

Assigning @abidart and @daredevil3435 to split the unitaryHACK bounty for this issue. Thanks again to both of you for your contribution to solve this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Development

Successfully merging a pull request may close this issue.

3 participants