Quandela / Perceval

An open source framework for programming photonic quantum computers
https://perceval.quandela.net
Other
133 stars 63 forks source link

QASM to linear optics circuit converter #399

Closed ericbrts closed 2 weeks ago

ericbrts commented 1 month ago

Taking QiskitConverter / MyQLMConverter as examples (located in package perceval.converter), write a converter taking a qasm file as input.

The QASMConverter will return a fully setup Perceval Processor. It needs to support a subset of gates that Perceval already supports (all 1-qubit gates, CX, CZ, CCX and CCZ), qubit naming. An exception should be raised for other gates, for classical bits, and other feature that Perceval does not support.

burlemarxiste commented 1 month ago

Is a dependency on https://github.com/QuTech-Delft/libqasm ok?

ericbrts commented 1 month ago

Hi @burlemarxiste ,

Thanks for your message.

From what I could check, yes it's completely OK. Could you put it in the extras_require of setup.py?

burlemarxiste commented 1 month ago

Hello @ericbrts!

I'll look into this on Monday! I'd like to wrap up the text rendering and barrier() submissions first, could I have your feedback on my questions before I push my code.

maxwell04-wq commented 1 month ago

Hi @ericbrts, Can you please confirm that the issue has indeed been assigned to someone else already and I am not to proceed further with the PR? As per the rules of UnitaryHack communicated via Discord, only the first successful PR needs to be merged and the issue cannot be assigned to anyone beforehand.

In that case, I'll also remove my PR to avoid my work being used by others.

burlemarxiste commented 1 month ago

I'm very sorry about the clash on this issue.

I'm trying to do things right (latest version of the cQASM syntax, use of an AST, error reporting, use of optical processing elements as opposed to unitaries, unit tests), so this takes extra time. They matter to me since the code is going to be merged into a public, production codebase.

ericbrts commented 1 month ago

Hi @maxwell04-wq , hi @burlemarxiste ,

Oh, my mistake. I saw a PR related to this issue and as @burlemarxiste stated she'd work on it since May 29, I just assumed it was the corresponding PR. Thanks to both of you for having cleared up the situation with good manners. And again, I apologize for having missed the incoming clash. I'm going to have to ask for Unitary Hack organizers guidance on this. I'll let you know asap.

Thanks, Eric

burlemarxiste commented 1 month ago

What about this solution: award the prize money to @maxwell04-wq because she was the first, maybe mark us as co-contributors if there is a leaderboard somewhere, and consider my submission for the codebase once it's done since it supports the most recent version (v3) of the standard and is thoroughly unit-tested?

I'd add, however, that after researching all the confusing "quantum IR" standards, it seems that OpenQASM is closer to being the standard. In that case, there is nothing to implement since it comes with qiskit and thus the existing qiskit_converter can handle the result.

Or maybe nothing is done at all because the qiskit converter does not map gates to meaningful dual-rail optical elements (eg, a PERM to implement the gate X), it's only instantiating "opaque" unitaries which tell us nothing about optics. I was on my way to rewrite the base AGateConverter to use such elements as often as possible, if this is something you'd like to see in the codebase, I can spend a day or two on this.

maxwell04-wq commented 1 month ago

@burlemarxiste maybe we can work as co-contributors and split the prize money? I'm perfectly fine with that.

And I agree with you, openqasm files do not need a dedicated converter as Qiskit can handle them. However, cQASM files are another case, and I could not find a parser that can effectively deal with cQASM files. I have worked on the implementation of CQASM version 1 and we can probably merge our code.

burlemarxiste commented 1 month ago

Here is my submission:

https://github.com/Quandela/Perceval/commit/1aae9407a7a81af37822ec9af8d084e790f7a7f9

In particular, all gates are converted to actual optical elements, and I believe the code here could be retrofit to AGateConverter to get the same kind of meaningful output from qiskit circuits.

It's in the same branch as the one in which I worked on the rendering fixes, I can create a single pull request with everything!

@ericbrts , what are the next steps?

ericbrts commented 1 month ago

First, thanks to both of you for having proposed a split bounty. I've contacted a UnitaryFund member, earlier this afternoon, to make sure if it's possible and how to make this work. Maybe you want to know the answer before continuing(?)

Me and my colleagues will check both your contributions this week. The first thing I'd say, is that we'd require unit tests before we accept a contributions.

Also @burlemarxiste , yes please, it would be easier for us if you could create one PR per issue (so, one branch per issue).

burlemarxiste commented 1 month ago

I had an idea.

https://github.com/Quandela/Perceval/commit/92179803d70953668e34fcd5cbe4af8600fa970a

I adapted the code from @maxwell04-wq to make it create a cQASM v3 AST. That way, the rest of my gate generation code can be used, and we can now handle both v1 and v3 files.

However, @maxwell04-wq 's parser needs quite some work to support the full syntax and be more robust to syntax errors. @maxwell04-wq, could you work on it? You can write more tests in the style of test_converter_v1 and see if they produce correct output. The cQASM syntax is simple, but I don't think you'll get robust code out of string manipulation. What about using a parsing library like pyparsing?

maxwell04-wq commented 1 month ago

Sure, I can work on it.

burlemarxiste commented 1 month ago

On my side, I'll split all my work in branches.

I still think that there should be a central dictionary of classic QC gates -> basic Circuit() shared by all converters to avoid opaque unitaries.

@ericbrts: what's your cutoff for a submission (time and perfectionism)?

Another question to @ericbrts : does gate synthesis from simpler gates (eg: CCNOT from CNOT, controlled-U from CNOT) belong somewhere in the codebase? I can implement something locally in AGateConverter to make it handle controlled rotation and thus convert more circuits from all existing Converters, but would it be relevant somewhere else too? I understand that controlled gates are not straightforward at all with optical circuits, so maybe synthesizing CCC-Rx is not very useful 😄 .

maxwell04-wq commented 1 month ago

@burlemarxiste based on the implementation in qiskit_converter and myqlm_converter, are we not to restrict our implementation to <=2-qubit gates?

maxwell04-wq commented 1 month ago

@ericbrts, do you suggest that @burlemarxiste and I work on separate cQASM converters, like in Qiskit?

burlemarxiste commented 1 month ago

@burlemarxiste based on the implementation in qiskit_converter and myqlm_converter, are we not to restrict our implementation to <=2-qubit gates?

My question is not about this specific issue, but in general... If I want to implement other 2-qubit or n-qubit gates to improve conversion in general, should I keep it local to Converter or would it be useful somewhere else?

ericbrts commented 1 month ago

Hello,

Just to be sure, UnitaryHack organizers answered that it's possible to split a bounty. Do you still agree on this @burlemarxiste and @maxwell04-wq ? If so, let's go for it, I assign the issue to both of you.

@ericbrts, do you suggest that @burlemarxiste and I work on separate cQASM converters, like in Qiskit?

No I was thinking about only one, of course. But as it seems you both already wrote some code, we'd need to review both and maybe cherry pick what we like - as long as it's not too inconvenient for the both of you.

@ericbrts: what's your cutoff for a submission (time and perfectionism)?

We'd like generic, readable and tested code to be integrated. That being said, working with external contributors we tend to accept different "forms" for the code. For this specific task, as it's stated you should inherit from a specific abstract class, it's fixing the architecture, at least.

does gate synthesis from simpler gates (eg: CCNOT from CNOT, controlled-U from CNOT) belong somewhere in the codebase? I can implement something locally in AGateConverter to make it handle controlled rotation and thus convert more circuits from all existing Converters, but would it be relevant somewhere else too? I understand that controlled gates are not straightforward at all with optical circuits, so maybe synthesizing CCC-Rx is not very useful 😄 .

I suggest we remain on exactly the features from QiskitConverter (i.e. raising an error on 3+ qubit gates). We already have a linear optics implementation for CCX and CCZ, and controlled-rotation implementation is coming in the next Perceval (0.11.0). You can check all that's available in pcvl.catalog. So we'll be able to add them to the converters layers later.

burlemarxiste commented 1 month ago

I agree to the bounty split.

No I was thinking about only one, of course

Just to be clear on that: my submission is for the latest version of the syntax, for which there is an officially supported parsing library; and @maxwell04-wq is for the v1, documented on the site you linked to.

So they both have their place!

@maxwell04-wq suggests to create two independent converters, the same way Qiskit has qiskit.qasm2 and qiskit.qasm3 for example.

My proposal is to have one CQASMConverter class with separate methods for loading v1 and v3 files (v2 seems to be experimental only); the v1 parser generating an AST compatible with v3 so that the circuit generation is shared between the two.

I suggest we remain on exactly the features from QiskitConverter

I just added a simple CRz for the sake of covering everything described in the cQASM v3 standard.

maxwell04-wq commented 1 month ago

@ericbrts I'm fine with splitting the bounty too. Can you please share your thoughts on this comment by @burlemarxiste :

My proposal is to have one CQASMConverter class with separate methods for loading v1 and v3 files (v2 seems to be experimental only); the v1 parser generating an AST compatible with v3 so that the circuit generation is shared between the two.

ericbrts commented 1 month ago

Hi, thanks for both your answers. You're now both assigned to this issue and UnitaryHack will split the corresponding bounty.

With a colleague, we've started reviewing both your branches.

We agree with @burlemarxiste proposal of having only one class handling both versions. Actually after the review, we'd like to go on with this PR : https://github.com/Quandela/Perceval/pull/407 because we think it's closer to the expected, and we'd like to optimize back and forth comments with the both of you. @raksharuia will publish a detailled review inside the PR as soon as it's ready.

Thanks, Eric

maxwell04-wq commented 3 weeks ago

@burlemarxiste can you please add me to your repo so that I can add the changes for cQASM v1?

burlemarxiste commented 3 weeks ago

I have added you, the changes are in the branch cqasm_converter

raksharuia commented 3 weeks ago

Hello @maxwell04-wq and @burlemarxiste, I am glad you resolved the issue of assigning @maxwell04-wq to the repo to allow both of you to work on the requested changes. However, FYI I am unavailable until Wed to review your changes in the PR. I will be happy to answer any questions if you have until then. :-)

ericbrts commented 2 weeks ago

Done and merged