Create glorious_folding_amplifier.py #36

mrvee-qC-bee · 2023-01-24T19:02:53Z

refactor(folding_amplifier): init dag folding amplifier

Summary

Details and comments

pedrorrivero

I have left some comments, once you're done with them try changing copies for num_foldings and appending two circuits per num_folding instead of just one as you are doing with copies. You should always retain the original circuit too, so this should give you something like:

num_foldings=0 → output circuit is the same as input circuit (i.e. once)
num_foldings=1 → output circuit is input circuit repeated three times
num_foldings=2 → output circuit is input circuit repeated five times

Remember to keep inserting barriers between each copy! If you feel brave enough, try to invert the first of those two circuits that you append afterwards 😉

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero

This is very good! Congrats!

Some general comments:

Try to do one thing only per commit (i.e. avoid something: this + that)
Try to write the scope in the commit message (e.g. refactor(folding_amplifier): update ...)

Other than that, great job! Try to act on the new comments and let's pick it up from there 🥳

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero · 2023-01-29T14:25:27Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+def _inverse_dag(dag_to_inverse: DAGCircuit) -> DAGCircuit:
+    """Inverts dag circuit"""
+    dag_to_inverse = dag_to_circuit(dag_to_inverse).inverse()
+    return circuit_to_dag(dag_to_inverse)


This is a good utility function! I am surprised that this does not exist natively in terra. We may want to contribute it (there is a similar method called DAGCircuit.reverse with an implementation just like yours).

Before doing that think about the following question: How many times do you traverse the entire circuit with this implementation? (Hint: look instruction by instruction and add up)

Once you respond to that, try to respond to the same question but for a piece of code which traverses the DAGCircuit in reverse order (i.e. form OutNodes to InNodes) adding the inverse gates as it encounters them.

Which one is (roughly) more efficient? (i.e. traverses the circuit less times and therefore is faster)

No need to update the code, but something to be aware of 😉

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero

Good, good, good!! This is great!!

Try to keep all comments related to Terra contributions UNresolved so that we remember to do those at some point 🙂

pedrorrivero · 2023-01-29T16:33:01Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+
+def apply_folding(original_dag: DAGCircuit, num_foldings: int) -> DAGCircuit:
+    """Build dag circuit by composing copies of an input dag circuit."""
+    noisy_dag = copy.deepcopy(original_dag)


Again, greatly surprised that there is no native DAGCircuit.copy method. We may need to contribute that to Terra as well. 😮

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero

Interesting approach! See comments 🙂

pedrorrivero · 2023-01-30T06:54:41Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+    dag_to_inverse = dag_to_circuit(dag_to_inverse)
+    dag_to_inverse.barrier()
+    dag_to_inverse = dag_to_inverse.inverse()
+    dag_to_inverse.barrier()


Uhh, I had not though of this! Interesting!

You should refrain from this approach for several reasons, but probably the most important is that you are not doing what the function promises. Instead you are doing extra things (i.e. adding two barriers) that are not expected, this is sometimes referred to with the broader term side effect which good code tries to avoid at all cost, as it makes it harder to understand and predict.

How about something like:

barrier = Barrier(noisy_dag.num_qubits()) ... for _ in range(num_foldings): noisy_dag.apply_operation_back(barrier, qargs=noisy_dag.qubits) noisy_dag.compose(inverse_dag, inplace=True) ...

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

Initial function

pedrorrivero

Great job!! Some clean-up to do, and some puzzles to solve, but you are making progress sir! 🎈

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero · 2023-01-31T21:19:05Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+        inverse_dag = self._invert_dag(dag)
+        for _ in range(num_foldings):
+            noisy_dag.apply_operation_back(barrier, qargs=noisy_dag.qubits)
+            noisy_dag.compose(inverse_dag, inplace=True)
+            noisy_dag.apply_operation_back(barrier, qargs=noisy_dag.qubits)
+            noisy_dag.compose(dag, inplace=True)


Suggested change

inverse_dag = self._invert_dag(dag)

for _ in range(num_foldings):

noisy_dag.apply_operation_back(barrier, qargs=noisy_dag.qubits)

noisy_dag.compose(inverse_dag, inplace=True)

noisy_dag.apply_operation_back(barrier, qargs=noisy_dag.qubits)

noisy_dag.compose(dag, inplace=True)

inverse_dag = self._invert_dag(original_dag)

for _ in range(num_foldings):

noisy_dag.apply_operation_back(barrier, qargs=noisy_dag.qubits)

noisy_dag.compose(inverse_dag, inplace=True)

noisy_dag.apply_operation_back(barrier, qargs=noisy_dag.qubits)

noisy_dag.compose(original_dag, inplace=True)

This seems to fail lint! The DAGNoiseAmplifier has dag as the variable name than original_dag :(

pedrorrivero · 2023-01-31T21:23:50Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+    def _apply_full_folding(
+        self,
+        noisy_dag: DAGCircuit,
+        dag: DAGCircuit,
+        barrier: Barrier,
+        num_foldings: int,
+    ) -> DAGCircuit:


Suggested change

def _apply_full_folding(

self,

noisy_dag: DAGCircuit,

dag: DAGCircuit,

barrier: Barrier,

num_foldings: int,

) -> DAGCircuit:

def _apply_full_folding(

self,

noisy_dag: DAGCircuit,

original_dag: DAGCircuit,

barrier: Barrier,

num_foldings: int,

) -> DAGCircuit:

A few things to notice i this signature:

As mentioned before, why do you need barrier as an arg? Are you going to be passing anything different any time?

why do we pass noisy_dag? I know you drew inspiration from the other class, but do you understand the reasons behind this? If not, there is no need to rush it, let's take it one step at a time and re-discover these things as we go 🙂 Your code really does not require that you pass a noisy_dag, better to provide the orginal_dag and num_foldings and return a freshly created noisy_dag for now.

Keep naming consistent, original_dag is better than just dag because it emphasizes the difference with noisy_dag

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero

This is starting to look very professional!! 💪🏼

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero · 2023-02-04T09:50:51Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+        inverted_dag = dag_to_inverse.copy_empty_like()
+        for node in dag_to_inverse.topological_op_nodes():
+            inverted_dag.apply_operation_front(
+                node.op.inverse(), qargs=node.qargs, cargs=node.cargs
+            )
+        return inverted_dag


SUPER COOL!! You wrote this using only the DAG API, which makes the code more efficient and, ready to be contributed to Terra!! Let's take care of this in our next chat 🥳

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero

Docstrings look good! Just a few comments, you are ready to move to the next step 🥳

Rename this class so it is clear this one is global and create a new class for the local amplifier! Avoid looking at the previous code, try to figure this one out on your own 😉

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero

Some details 🙂

zne/noise_amplification/folding_amplifier/glorious_global_folding_amplifier.py

pedrorrivero

Good job!! 🎉

I left some remarks to get you thinking about more advanced topics on software design. Let me know what you think 😉

zne/noise_amplification/folding_amplifier/glorious_global_folding_amplifier.py

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

pedrorrivero · 2023-02-08T19:15:18Z

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

+                noisy_dag = self._insert_folding(noisy_dag, node, num_foldings)
+            else:
+                noisy_dag.apply_operation_back(node.op, qargs=node.qargs, cargs=node.cargs)


How about we try to mimic the language that is already in place to make the code more readable? 🙃

Suggested change

noisy_dag = self._insert_folding(noisy_dag, node, num_foldings)

else:

noisy_dag.apply_operation_back(node.op, qargs=node.qargs, cargs=node.cargs)

noisy_dag = self._apply_folded_operation_back(noisy_dag, node, num_foldings)

else:

noisy_dag.apply_operation_back(node.op, qargs=node.qargs, cargs=node.cargs)

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

pedrorrivero · 2023-02-08T22:43:34Z

There is one more thing that we could do here: add some configuration to the amplifiers that controls whether barriers are applied or not. 🙃

pedrorrivero

Great progress!! Let's discuss, and keep it up! 🥳

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero · 2023-05-16T13:08:35Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+
+from ..noise_amplifier import DAGNoiseAmplifier
+
+Folding = namedtuple("Folding", ("full", "partial", "effective_noise_factor"))


This is very good!

We should maybe rethink the language at some point according to that glossary that I sent you a while back, so that terminology is a bit clearer 🙂

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

pedrorrivero · 2023-05-16T13:14:48Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+            Folding: named tuple containing full foldings, number of gates to partially fold and
+            effective noise factor of the operation
+        """
+        noise_factor = self._validate_noise_factor(noise_factor)


Since this is an internal function you do not need to perform validation and it should be avoided unless strictly necessary. Instead, assume that the input is correct (after all it will be you passing it along) and move validation to the point where the actual user of the tool passes noise_factors only 🙂

Tried something on those lines! 🥳

pedrorrivero · 2023-05-16T13:17:13Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+        )
+        return Folding(full, partial, effective_noise_factor)
+
+    def _compute_best_estimate(self, num_nodes: float, partial_foldings: float) -> float:


This name is not explanatory: if I am new to the tool and I read it, I would not know what is this a best estimate of!

Naming is tough!! 😅

Done! (sorta!) 🥳

pedrorrivero · 2023-05-16T13:56:56Z

zne/noise_amplification/folding_amplifier/glorious_global_folding_amplifier.py

+        if self.barriers:
+            barrier = Barrier(dag.num_qubits())
+            dag.apply_operation_back(barrier, qargs)
+
    def _invert_dag(self, dag_to_inverse: DAGCircuit) -> DAGCircuit:


Simple and elegant function! I love it 😍

TODO: Contribute to terra

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

pedrorrivero · 2023-05-16T13:59:30Z

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

@@ -33,99 +32,74 @@ class GloriousLocalFoldingAmplifier(DAGNoiseAmplifier):
            `<https://ieeexplore.ieee.org/document/9259940>`
    """

-    def __init__(self, gates_to_fold: Sequence[int | str], barriers: bool = True) -> None:
+    def __init__(self, gates_to_fold: Set[Union[str, int] | None], barriers: bool = True) -> None:
        self.gates_to_fold = self._validate_gates_to_fold(gates_to_fold)


Validation should happen in the setter —you currently don't have one, add it! 👨🏼‍💻

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

pedrorrivero · 2023-05-16T14:03:22Z

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

        VALID_GATES = set(  # pylint: disable=invalid-name
            standard_gates.get_standard_gate_name_mapping().keys()
        )


How do we handle custom gates? 🤔

Will ask a query in our next meet! :D

…lifier.py Co-authored-by: Pedro Rivero <[email protected]>

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

mrvee-qC-bee · 2023-05-30T14:33:56Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+        full_foldings = int(foldings)
+        partial_foldings = foldings - full_foldings
+
+        gates_to_partial_fold = self._compute_best_estimated_gates(num_nodes, partial_foldings)


Change to num_partial_folds

I have had an idea on how to make this even cleaner, let's discuss on Tuesday! 🙂 It is based on computing the total number of foldings to perform in the whole circuit directly, and choosing how that gets distributed later

mrvee-qC-bee · 2023-05-30T14:36:36Z

zne/noise_amplification/folding_amplifier/glorious_global_folding_amplifier.py

@@ -31,27 +31,16 @@ class GloriousGlobalFoldingAmplifier(GloriousFoldingAmplifier):
            `<https://ieeexplore.ieee.org/document/9259940>`
    """

-    def __init__(self, barriers: bool = True) -> None:
-        self._set_barriers(barriers)
+    def __init__(self) -> None:


Add barrier and tolerance

mrvee-qC-bee · 2023-05-30T14:37:34Z

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

+    ) -> None:
+        super().__init__()
+        self._set_custom_gates(custom_gates)
+        self._set_gates_to_fold(gates_to_fold)


Add barriers and tolerance

pedrorrivero

Good, let's push this through the finish line! :) The PR is getting quite substantial, it might be good to split it in smaller chunks. Let's consider this in our next chat!

pedrorrivero · 2023-06-04T19:20:06Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+    ################################################################################
+    @property
+    def barriers(self) -> bool:
+        """Barriers setter"""


This is actually a getter not a setter, but it is also good if you explain what the property means (e.g. if set to True, barriers are added when folding) 🙂

pedrorrivero · 2023-06-04T19:20:24Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+        return self._barriers
+
+    def _set_barriers(self, barriers: bool) -> None:
+        """Set barriers property"""


This is a setter on the other hand!

pedrorrivero · 2023-06-04T19:21:37Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+
+    @property
+    def tolerance(self) -> float:
+        """Tolerance setter"""


Again, not a setter, but it is better to explain the property in this docstring. It is somewhat eveident tht this is a getter so writing it on the docstring is not adding any relevant information 🙃

pedrorrivero · 2023-06-04T19:22:03Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+        return self._tolerance
+
+    def _set_tolerance(self, tolerance: float) -> None:
+        """Set Tolerance property"""


This is the real tolerance setter

Suggested change

"""Set Tolerance property"""

"""Set tolerance property"""

pedrorrivero · 2023-06-04T19:27:56Z

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py

+        full_foldings = int(foldings)
+        partial_foldings = foldings - full_foldings
+
+        gates_to_partial_fold = self._compute_best_estimated_gates(num_nodes, partial_foldings)


I have had an idea on how to make this even cleaner, let's discuss on Tuesday! 🙂 It is based on computing the total number of foldings to perform in the whole circuit directly, and choosing how that gets distributed later

pedrorrivero · 2023-06-04T19:30:40Z

zne/noise_amplification/folding_amplifier/glorious_global_folding_amplifier.py

    ################################################################################
    ## INTERFACE IMPLEMENTATION
    ################################################################################
    def amplify_dag_noise(self, dag: DAGCircuit, noise_factor: float) -> DAGCircuit:
        """Applies global folding to input DAGCircuit and returns amplified circuit"""
+        noise_factor = self._validate_noise_factor(noise_factor)


GREAT! 👏🏼

pedrorrivero · 2023-06-04T19:31:33Z

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

+    @property
+    def custom_gates(self) -> bool:
+        """Custom_gates flag setter"""
+        return self._custom_gates
+
+    def _set_custom_gates(self, custom_gates: bool) -> None:
+        """Set gates_to_fold property"""
+        self._custom_gates = bool(custom_gates)


What is this for? 🤔

TODO: Remove entirely

pedrorrivero · 2023-06-04T19:32:10Z

zne/noise_amplification/folding_amplifier/glorious_local_folding_amplifier.py

+        if self.custom_gates:
+            pass
+            # TODO: Append valid custom_gate_names to VALID_GATES


Can you explain this further? Thanks! 🙂

pedrorrivero reviewed Jan 24, 2023

View reviewed changes

pedrorrivero reviewed Jan 29, 2023

View reviewed changes

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py Outdated Show resolved Hide resolved

pedrorrivero reviewed Jan 29, 2023

View reviewed changes

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py Outdated Show resolved Hide resolved

pedrorrivero reviewed Jan 29, 2023

View reviewed changes

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py Outdated Show resolved Hide resolved

pedrorrivero reviewed Jan 29, 2023

View reviewed changes

pedrorrivero reviewed Jan 30, 2023

View reviewed changes

mrvee-qC-bee added 6 commits February 1, 2023 03:40

Create glorious_folding_amplifier.py

7911e36

Initial function

refactor: modify structure + add inverse copy

af082ea

refactor(folding_amplifier): make apply_folding a bit efficient

30ee3b6

docs(folding_amplifier): Add module docstring

4230d01

refactor(folding_amplifier): make apply_folding little bit efficient

415cb06

refactor(folding_amplifier): make apply_folding a bit memory efficient

2effe60

mrvee-qC-bee force-pushed the dagamplify branch from 60f71f2 to d10067b Compare January 31, 2023 19:41

refactor(folding_amplifier): isolate GloriousFoldingAmplifier class

9927cbc

mrvee-qC-bee force-pushed the dagamplify branch from d10067b to 9927cbc Compare January 31, 2023 20:39

pedrorrivero reviewed Jan 31, 2023

View reviewed changes

mrvee-qC-bee added 6 commits February 2, 2023 23:35

refactor(folding_amplifier): restructure GloriousFoldingAmplifier class

624ff62

docs(folding_amplifier): fix grammar

c074e01

docs(folding_amplifier): Add TODO

8ae67b7

refactor(folding_amplifier): add general typing for noise_factor

2742079

docs(folding_amplifier): add better exception message

68583f2

refactor(folding_amplifier): make invert function glorious

2303465

pedrorrivero reviewed Feb 4, 2023

View reviewed changes

mrvee-qC-bee added 4 commits February 6, 2023 00:42

docs(folding_amplifier): make docs a bit nicer

78dc662

refactor(folding_amplifier): refactor typing and exception handling

f3e03b4

docs(folding_amplifier): fix grammar

746afd4

docs(folding_amplifier): make spacing consistent

efaa007

pedrorrivero reviewed Feb 5, 2023

View reviewed changes

refactor(folding_amplifier): refactor typing and exception handling

63fd450

mrvee-qC-bee added 2 commits February 6, 2023 01:46

docs(folding_amplifier): rename for distinction

1329252

docs(folding_amplifier): rename for distinction

3401b1f

pedrorrivero reviewed Feb 6, 2023

View reviewed changes

mrvee-qC-bee added 2 commits February 7, 2023 22:33

docs(folding_amplifier): fix lint and typos

73ab3d6

feat(folding_amplifier): add glorious local folding amplifier

ff175fe

pedrorrivero reviewed Feb 8, 2023

View reviewed changes

mrvee-qC-bee added 2 commits February 14, 2023 21:56

refactor(folding_amplifier): improve validate_gates_to_fold

c66f2aa

refactor(folding_amplifier): add glorious_folding_amplifier interface

98f0039

pedrorrivero assigned mrvee-qC-bee Mar 2, 2023

pedrorrivero added enhancement New non-feature request (e.g. performance) PL-3 Priority level 3/5 → Medium labels Mar 2, 2023

mrvee-qC-bee added 8 commits March 13, 2023 00:15

refactor(folding_amplifier): clean glorious_global_folding amplifier

a2575ff

docs(folding_amplifier): make error messages concise

2689957

docs(folding_amplifier): remove typing from docstring

1baf3f1

refactor(folding_amplifier): add naive support for partial folding

b569ea3

docs(folding_amplifier): add doc strings to newer functions

b1fb40d

docs(folding_amplifier): remove comments

aa77062

refactor(folding_amplifier): draft masking function to be ported

cf5b96c

refactor(folding_amplifier): make globalfolding refactor functional

5bc8afb

pedrorrivero requested changes May 16, 2023

View reviewed changes

Update zne/noise_amplification/folding_amplifier/glorious_folding_amp…

7db66a7

…lifier.py Co-authored-by: Pedro Rivero <[email protected]>

pedrorrivero reviewed May 16, 2023

View reviewed changes

zne/noise_amplification/folding_amplifier/glorious_folding_amplifier.py Outdated Show resolved Hide resolved

feat(glorius_folding_amplifier): fix imports and inits

31054ce

mrvee-qC-bee commented May 30, 2023

View reviewed changes

pedrorrivero requested changes Jun 4, 2023

View reviewed changes


		from ..noise_amplifier import DAGNoiseAmplifier

		Folding = namedtuple("Folding", ("full", "partial", "effective_noise_factor"))

Create glorious_folding_amplifier.py #36

Are you sure you want to change the base?

Create glorious_folding_amplifier.py #36

Conversation

mrvee-qC-bee commented Jan 24, 2023 • edited Loading

Summary

Details and comments

pedrorrivero left a comment

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

pedrorrivero Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

pedrorrivero Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrorrivero commented Feb 8, 2023

pedrorrivero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrorrivero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrvee-qC-bee commented Jan 24, 2023 •

edited

Loading

pedrorrivero Jan 29, 2023 •

edited

Loading

pedrorrivero Jan 29, 2023 •

edited

Loading