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

Error when FreeParameters are named QASM types #603

Closed
mbeach-aws opened this issue Jul 6, 2023 · 17 comments · Fixed by #999
Closed

Error when FreeParameters are named QASM types #603

mbeach-aws opened this issue Jul 6, 2023 · 17 comments · Fixed by #999
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@mbeach-aws
Copy link
Contributor

Describe the bug
When a user defined a FreeParameter("str") where the "str" is a OpenQASM keyword. For example, it fails for b, q, bit, qubit, input, OPENQASM, 3.0. 1,2.
To reproduce

from braket.circuits import Circuit, FreeParameter
from braket.devices import LocalSimulator

b = FreeParameter("b")

circ = Circuit().rx(0,b)
print(circ)

device = LocalSimulator()

task = device.run(circ, shots=1_000, inputs={"b": 0})
task.result().measurement_counts

gives the error

    221     return builtin_constants[node.name]
    222 if not self.context.is_initialized(node.name):
--> 223     raise NameError(f"Identifier '{node.name}' is not initialized.")
    224 return self.context.get_value_by_identifier(node)

NameError: Identifier 'b' is not initialized.

Expected behavior
Expected the circuit to run correctly. For example, using FreeParameter("a") works correctly.

@mbeach-aws mbeach-aws added the bug Something isn't working label Jul 6, 2023
@ajberdy
Copy link
Contributor

ajberdy commented Jul 6, 2023

For context, here is the OpenQASM generated when an input is named b

OPENQASM 3.0;
input float b;
bit[1] b;
qubit[1] q;
rx(b) q[0];
b[0] = measure q[0];

@guomanmin
Copy link
Contributor

b and q are used as variable names by the Braket SDK and will cause a collision if customers create parameters with the same names. To mitigate this, we will update these automatically created variables to __bits__ and __qubits__.

The other variable names listed conflict with OpenQASM naming standards and we will update documentation to reflect this.

@rmshaffer
Copy link
Contributor

Reopening this issue, as PR #675 was reverted by PR #701. Additionally, I think there is still a need to document the fact that OpenQASM language-reserved words (such as input, angle, etc.) are not allowed as FreeParameter names. Ideally this would give an error message but at the very least it should be documented (e.g. in the FreeParameter documentation).

@rmshaffer rmshaffer reopened this Oct 4, 2023
@laurencap laurencap removed the good first issue Good for newcomers label Jan 29, 2024
@laurencap
Copy link
Contributor

I don't see on #701 the reason for reverting. Anyone know what the issue was?

@rmshaffer
Copy link
Contributor

I don't see on #701 the reason for reverting. Anyone know what the issue was?

I believe there was an issue with a particular OpenQASM parser not properly handling variable names beginning with __, which essentially caused it to be a breaking change when using that parser.

@mbeach-aws
Copy link
Contributor Author

Any progress on this issue? @kshitijc

@rmshaffer rmshaffer added the good first issue Good for newcomers label May 14, 2024
@Tarun-Kumar07
Copy link
Contributor

Tarun-Kumar07 commented Jun 4, 2024

Hi @rmshaffer and @mbeach-aws , I would like to tackle this issue as part of unitary hack.

@Tarun-Kumar07
Copy link
Contributor

Hi @rmshaffer and @mbeach-aws,

I came up with two solutions, please let me know which I should proceed with.

Solution 1

  • We always prefix FreeParameter, e.g., FreeParameter("b") => fp_b.
  • If we use this, we can skip the validation of using OpenQASM language-reserved keywords as there would be no collision.
  • Here, users will have more flexibility in providing the name.

Solution 2

  • As done in #675, we can use different names for q and b:
    • q => sdk_total_qubits
    • b => sdk_total_bits
  • Here, we would need to document not to use OpenQASM language-reserved keywords and the above two mentioned variable names to avoid collisions.
  • Here, users would have less flexibility in providing the name.

@rmshaffer
Copy link
Contributor

@Tarun-Kumar07 I believe we should do something more like your Solution 1, but only when FreeParameter matches one of the OpenQASM reserved words, or one of the SDK predefined variables like b or q - I don't believe there is a need to modify all variable names in the program. An alternative would be to output a useful error message in these cases telling the user to change their variable name.

@Tarun-Kumar07
Copy link
Contributor

@rmshaffer, is it guaranteed that SDK would only have predefined variables b or q ?
If we introduce some new predefined variable, there is a potential chance we might break existing user code if they already use the same variable.
If we prefix all the user defined variables, then there won't be collision, but we have to ensure we are not using the prefix while generating SDK predefined variables.

@rmshaffer
Copy link
Contributor

I don't believe there would be any reason for the SDK to introduce new predefined variables beyond b and q. If so, this would need to be accounted for at that time, but I think that is beyond the scope of this issue.

@Tarun-Kumar07
Copy link
Contributor

@rmshaffer , I will go with the solution where we are prefixing predefined variables and OpenQASM reserved words

@Tarun-Kumar07
Copy link
Contributor

Hi @rmshaffer,

I decided to output useful error messages instead of prefixing inputs because when specifying inputs in device.run, I would also need to append the prefix.

I made a list of reserved keywords from the OpenQASM grammar. I only included words like float, angle, etc., and ignored symbols like =, ;, etc., as users are more likely to enter words.

Currently, I expect the following code snippet to pass, but it unexpectedly breaks for four reserved keywords: dim, cal, void, and readonly.

QASM_RESERVED_WORDS = [
    "OPENQASM", "include", "defcalgrammar", "def", "cal", "defcal", "gate", "extern",
    "box", "let", "break", "continue", "if", "else", "end", "return", "for", "while",
    "in", "pragma", "input", "output", "const", "readonly", "mutable", "qreg", "qubit",
    "creg", "bool", "bit", "int", "uint", "float", "angle", "complex", "array", "void",
    "duration", "stretch", "gphase", "inv", "pow", "ctrl", "negctrl", "dim", "durationof",
    "delay", "reset", "measure", "barrier", "true", "false"
]

@pytest.mark.parametrize("param", QASM_RESERVED_WORDS)
def test_bug(param):
    from braket.circuits import Circuit, FreeParameter
    from braket.devices import LocalSimulator

    b = FreeParameter(param)
    circ = Circuit().rx(0, b)
    print(circ)

    with pytest.raises(Exception):
        device = LocalSimulator()
        task = device.run(circ, shots=1_000, inputs={param: 0})
        task.result().measurement_counts

Could you please help me understand why this is happening?

@rmshaffer
Copy link
Contributor

Hi @Tarun-Kumar07 - could you please open a draft PR with your code so that we could more easily review your changes?

I don't know what would cause those four keywords to be different than the others. When you say "it unexpectedly breaks", what does that mean? Do you get an error message of some kind? Once you open a PR, if you could please comment in the PR with those details, that would be the easiest way for us to help you out.

Thank you for your investigation into this issue!

@Tarun-Kumar07
Copy link
Contributor

Hi @rmshaffer,

I've submitted PR #999 to address the issue. I would appreciate your feedback.

@Tarun-Kumar07
Copy link
Contributor

Hi, can this issue be assigned to me after merging #999.
Thanks !!

@rmshaffer
Copy link
Contributor

@Tarun-Kumar07 PR #999 has been merged. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants