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

Update the included OpenQASM grammar to OpenQASM 3.1.0 #24

Merged

Conversation

hodgestar
Copy link
Contributor

No description provided.

@hodgestar
Copy link
Contributor Author

hodgestar commented Jun 12, 2024

Tests pass for me with the main branch of the https://github.com/openqasm/openqasm/ repository but we'll probably need a new release before they pass here.

@hodgestar
Copy link
Contributor Author

@braised-babbage Would you mind having a very brief look at this and letting me know if there is anything obviously missing from it? E.g. Should tests be added for the new OpenQASM 3.1 features?

@braised-babbage braised-babbage self-requested a review June 26, 2024 16:14
Copy link
Collaborator

@braised-babbage braised-babbage left a comment

Choose a reason for hiding this comment

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

I took a look at this. A few thoughts

  1. I ran our internal regression tests on this, using the openqasm3 v1.0 release. Everything passed (but note that these tests do not use OpenQASM 3.1 language features).
  2. I think we should update src/openpulse/ANTLR_VERSIONS.txt to cover more recent versions supported by openqasm3 (see this list)
  3. Yes, I think it makes sense to add tests for new language features. I tried one for switch within a cal block and can see that it doesn't quite work yet:

parsing

    const int A = 0;
    
    cal {      
      int x = 10;
      switch (x) {
        case 1, 3, 5 {
          x += 1;
        }
        case 2, 4, 6 {
          x += 1;
          x *= 2;
        }
        case A {
        }
        default {
        }
      }
    }

and then printing this out (openpulse.printer.dumps) gives

const int A = 0;
cal {
  int x = 10;
}

This makes sense, given that there is no switchStatement option for openpulseStatement (in source/grammar/openpulseParser.g4). It's kind of annoying that these failures are silent though.

I suppose a part of the discussion around (3) is whether we should generally push new OpenQASM features into OpenPulse (to be usable within a cal block). I think for classical types & control flow, the answer should be yes -- OpenPulse is intended to be an extension of OpenQASM in this respect, and I can imagine something like switch being useful at a pulse level.

@hodgestar
Copy link
Contributor Author

@braised-babbage Would you mind doing another review?

Outstanding questions:

  • Should we update the OpenPulse __version__ in this PR or a later one? And should we update it to 1.0.0?
  • I haven't yet figured out why the Python 3.12 (and previously 3.10) tests are failing to find the built ANTLR packages.

@braised-babbage
Copy link
Collaborator

braised-babbage commented Jul 22, 2024

@braised-babbage Would you mind doing another review?

Outstanding questions:

  • Should we update the OpenPulse __version__ in this PR or a later one? And should we update it to 1.0.0?

Looks good to me. Thanks for doing this.

Re: the __version__, maybe we should decide what we want in a 1.0.0 release. Of the open issues

Note that some of the above PRs fail in CI because the current constraint on openqasm3 in requirements.txt is too lax (cf. #26 (comment))

@braised-babbage
Copy link
Collaborator

  • I haven't yet figured out why the Python 3.12 (and previously 3.10) tests are failing to find the built ANTLR packages.

That's strange -- your guess is as good as mine.

This was referenced Jul 23, 2024
@hodgestar
Copy link
Contributor Author

@braised-babbage Tests passing!

Copy link
Collaborator

@braised-babbage braised-babbage left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice idea

@braised-babbage braised-babbage merged commit 0f8aaeb into openqasm:main Jul 29, 2024
6 checks passed
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.

2 participants