@nelimee I am not sure why the CI/CD is failing.

Also, I am thinking if it is worth merging with the canonical form of the circuits expressed as RPNG strings, leaving the general interface for custom compilation for another PR.

Hi Gian,

I am not sure why the CI/CD is failing.

You need to install and run the pre-commit hooks locally. And the deploy failure is because you did not create the branch under the tqec repository but use your fork instead, it will cause permission issue.

Also, I am thinking if it is worth merging with the canonical form of the circuits expressed as RPNG strings, leaving the general interface for custom compilation for another PR.

In my opinion, they should be in separate PR.

A few preliminary notes on the current state of the implementation:

• The functions are too complex. There are too much if/else branches, too much "magic" (e.g., v[0] could be renamed r in most cases, to make it easier to read).
• I think that a dataclass with some methods would be perfect to wrap one RPNG representation. The dataclass would be responsible to validate that the provided RPNG string is locally valid (i.e. that we have correct values for R, P and G) and would provide methods to ease the global validation (e.g., a property n returning the timestep at which the 2-qubit gate is scheduled).
• The dataclass could also help for circuit generation, for example by returning the correct reset/measurement/computation gates using methods.

I am not sure why the CI/CD is failing.

You need to install and run the pre-commit hooks locally. And the deploy failure is because you did not create the branch under the tqec repository but use your fork instead, it will cause permission issue.

Would you suggest creating a new branch in the official tqec repo, merge my fork there, and then create a PR internal to tqec?
In general I would find it more convenient to work directly in the tqec repo, but didn't know if that's a good practice.

A few preliminary notes on the current state of the implementation:

• The functions are too complex. There are too much if/else branches, too much "magic" (e.g., v[0] could be renamed r in most cases, to make it easier to read).
• I think that a dataclass with some methods would be perfect to wrap one RPNG representation. The dataclass would be responsible to validate that the provided RPNG string is locally valid (i.e. that we have correct values for R, P and G) and would provide methods to ease the global validation (e.g., a property n returning the timestep at which the 2-qubit gate is scheduled).
• The dataclass could also help for circuit generation, for example by returning the correct reset/measurement/computation gates using methods.

Thanks for the early feedback, Adrien.

I can wrap the rpng strings into a dataclass to facilitate querying the values of interest.
Is the idea to facilitate writing custom circuits from the rpng?
Or do you think it would affect how the rest of the backend uses it?
(in my mind we just need the plaquette given a valid rpng string)

I'll include the validation inside that class, but would not mind keeping its "complex" implementation for the moment (there is no urgency here and no risk of computational slowdowns).

I can wrap the rpng strings into a dataclass to facilitate querying the values of interest.
Is the idea to facilitate writing custom circuits from the rpng?
Or do you think it would affect how the rest of the backend uses it?
(in my mind we just need the plaquette given a valid rpng string)

See for example #412. I think we need a simple and robust representation of a RPNG string, and a RPNGPlaquette, as we might have to use them in several places.

I'll include the validation inside that class, but would not mind keeping its "complex" implementation for the moment (there is no urgency here and no risk of computational slowdowns).

The problem is mostly about code reuse. I think it is better to have most of the validation code in one place, and #412 would need a little bit more flexibility, so I think we shoule have reusable structures if we can.

See for example #412. I think we need a simple and robust representation of a RPNG string, and a RPNGPlaquette, as we might have to use them in several places.

Thanks for the concrete example!
I adopted most of it (I am unfamiliar with the template part so I left it out) and the PR should be ready for review.

@giangiac the goal is to merge that PR as soon as possible to unlock other issues. I'll make a real full review, and we will be able to merge once the review will be addressed.

I would vote against the 5-character notation. In my mind if you need to go to a circuit that cannot be expressed with the four character notation, you probably wouldn't express that circuit using plaquettes in the first place. H can be on both reset and measurement as a typical expansion of cnot into CZ can easily require H on both ends of the circuit. I think reset time should always be zero, and the measurement time should be the latest time of a gate in an entire circuit layer plus one. Best, Austin.

I am catching up with the comments and it seems that the consensus (at least among the active users @afowler @inmzhang @nelimee ) is to focus exclusively on the 4-character notation.
I will move the 5-character one to separate classes (not to lose its implementation in case of future reevaluation).

Question on N: Is it ok to assume a single digit value or shall we provide the flexibility to have a larger integer?
This may happen when gates take more than a "time step", if we describe time in terms of a certain clock.

Question on the measurement time: We need to know that at the time when we generate the plaquette.
Are we going to populate the templates with Plaquette or RPNGDescription objects?

Question on N: Is it ok to assume a single digit value or shall we provide the flexibility to have a larger integer?

Let's have the simple case correct first. If we need 10+ values for N we will improve that afterwards.

Question on the measurement time: We need to know that at the time when we generate the plaquette.
Are we going to populate the templates with Plaquette or RPNGDescription objects?

Templates will return RPNG strings. These strings will be translated to Plaquette instances at circuit creation, when we will have all the global information for each round of error correction, which is needed to synchronise measurements.

Here is a proposal for the RPNG.from_string method that makes it more readable in my opinion. I am also fine with the existing code (modulus my comments), so it is up to preference.

    @staticmethod
    def _parse_or_raise(
        typ: type[RPNGCharType], char: str, pos: Literal["R", "P", "N", "G"]
    ) -> RPNGCharType:
        try:
            return typ(char)
        except ValueError as err:
            raise ValueError(
                f"Unacceptable character for the {pos} field: '{char}'."
            ) from err

    @classmethod
    def from_string(cls, rpng_string: str) -> RPNG:
        """Initialize the RPNG object from a 4-character string

        Note that any character different from a BasisEnum / ExtendedBasisEnum
        value would result in the corresponding field being None.
        """
        if len(rpng_string) != 4:
            raise ValueError("The rpng string must be exactly 4-character long.")
        r_str, p_str, n_str, g_str = tuple(rpng_string)
        # Convert the characters into the enum attributes (or raise error).
        r = RPNG._parse_or_raise(ExtendedBasisEnum, r_str, "R")
        p = RPNG._parse_or_raise(BasisEnum, p_str, "P")
        n = RPNG._parse_or_raise(int, n_str, "N")
        g = RPNG._parse_or_raise(ExtendedBasisEnum, g_str, "G")

        return cls(r, p, n, g)

The code

from tqec import RPNG

rpng = RPNG.from_string2("e---")

raises and prints to the screen

Traceback (most recent call last):
  File "/workspaces/tqec/src/tqec/plaquette/rpng.py", line 100, in _parse_or_raise
    return typ(char)
           ^^^^^^^^^
  File "/usr/local/lib/python3.12/enum.py", line 757, in __call__
    return cls.__new__(cls, value)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/enum.py", line 1171, in __new__
    raise ve_exc
ValueError: 'e' is not a valid ExtendedBasisEnum

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/workspaces/tqec/main.py", line 3, in <module>
    rpng = RPNG.from_string2("e---")
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/tqec/src/tqec/plaquette/rpng.py", line 117, in from_string2
    r = RPNG._parse_or_raise(ExtendedBasisEnum, r_str, "R")
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/tqec/src/tqec/plaquette/rpng.py", line 102, in _parse_or_raise
    raise ValueError(
ValueError: Unacceptable character for the R field: 'e'.

Here is a proposal for the RPNG.from_string method that makes it more readable in my opinion. I am also fine with the existing code (modulus my comments), so it is up to preference.

I am not sure how you defined RPNGCharType, but one has to include the desired behavior for - or it would not work.
What if we merge this code and leave code readability for later?

From my point of view, this is ready to be merged.

A few things left for future PRs:

1. a generic and documented interface, so that users will be able to implement their own translation if they want
2. expand the tqec documentation, for example by adding to the user_guide a new section about creating the plaquettes
3. improve code readability (see Adrien's proposal)

@giangiac do you want me to merge that PR, or are you waiting for something else to merge?

@giangiac do you want me to merge that PR, or are you waiting for something else to merge?

Usually, I let someone else merge my PRs, but it seems that the consensus is to merge.
So, I will go for it. :-)