[QHC-833] Improving digital Transpilation

[GuillermoAbadLopez]

TODOs for this PR:

(each of the next points is pretty big deal, so check them all please 🙌🙃)

• Move optimize flag, for actual optional optimizations (& Improve optimize word use in methods names)
• Make Transpilation/execute/compile only for single circuits (unify code standard across qililab)
• Make Transpilation efficient, by not constructing the Circuit class so many times, between methods
• Pass a transpilation kwargs instead of so many args in platform/qililab's execute(...)
• Improve documentation on transpilation, simplifying it in execute()'s, and moving it to a new section.
• Cover/change test for all of this.

---

You will see two empty methods, in this next PR I will fill them:

bunch_drag_gates(...), delete_gates_with_no_amplitude(...)

---

---

The documentation after this PR looks like:

For the platform.execute():

For the module about Transpilation:

For the Transpiler.transpile_circuit():

[pullpo-for-slack]

🔗Pullpo.io Slack channel

[pullpo-for-slack]

AI Analysis

Review:

The changes in circuit_transpiler.py to add phases from RZs and CZs gates to Drag gates, optimize transpilation by combining consecutive Drags, and adding explanatory comments seem well-structured and clear. It's great to see the addition of optimization methods for the transpilation process. In circuit_optimizer.py, the addition of the method to add phases to Drag gates and optimize transpilation by combining consecutive Drags aligns well with the changes made in circuit_transpiler.py. It's good to maintain consistency across the codebase.

Detailed file changes

(dropdown):

In circuit_transpiler.py:

• Added a method to add phases from RZs and CZs gates to Drag gates in the circuit.
• Modified the transpile_circuits method to use the new method to add phases to Drags.
• Added comments explaining the process of adding phases from RZs and CZs gates to Drags.
• Added a method to optimize transpilation by bunching consecutive Drag gates together into a single one.

In circuit_optimizer.py:

• Added a method to add phases from RZs and CZs gates to Drag gates in the circuit.
• Modified the cancel_pairs_of_hermitian_gates method signature to return a Circuit.
• Added a method to optimize transpilation by bunching consecutive Drag gates together into a single one.

[linear]

QHC-833 Transpilation feedback/improvements

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.78%. Comparing base (7226edf) to head (8b23588).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #862   +/-   ##
=======================================
  Coverage   96.77%   96.78%           
=======================================
  Files         221      221           
  Lines        7756     7765    +9     
=======================================
+ Hits         7506     7515    +9     
  Misses        250      250           

Flag	Coverage Δ
unittests	96.78% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fedonman]

Nice job! Left some comments.

[fedonman]

I don't like replacing the parameters with a generic dictionary. Now, it is not clear what the dictionary should contain, what are the default values, there is no autocomplete etc.

In general, having all options as separate parameters is preferred.

If we really do need to use a single parameter, we should use a NamedDict or a new class.

[fedonman]

Also, default parameter values should not be mutable: https://docs.astral.sh/ruff/rules/mutable-argument-default/

[GuillermoAbadLopez]

Also, default parameter values should not be mutable: https://docs.astral.sh/ruff/rules/mutable-argument-default/

Damn, true... I guess I'll have to set it back to None then... started like that, but that made the code much uglier, with more checks, that complicated it... 😅. But definitely can go back to it, makes sense 👌

[GuillermoAbadLopez]

I don't like replacing the parameters with a generic dictionary. Now, it is not clear what the dictionary should contain, what are the default values, there is no autocomplete etc.

In general, having all options as separate parameters is preferred.

If we really do need to use a single parameter, we should use a NamedDict or a new class.

Yeah I didn't like it either, but it was so many parameters, which might keep growing..., all of them related to the same thing, that in the end discussing it with @visagim we thought, it didn't make sense to keep the params separated, mixing with the other compile/execute parameters, when they clearly are more "modular" 😓. (That's also why I only did it for compile/execute's, but not for the transpile methods down the line)

I also tried first to do a default class, but what I didn;t like about it, is the user having to import and initialize a random class, from a file in qililab, dunno how intuitive that would be 😅. But its true that at least you would then have initialization.

Dunno I'm not 100% convinced what is better, I can try to generate the three examples:

• Keep params separated
• Ganeric kwargs
• Specific qililab dataclass

and ask again in a Code Discussion/daily, where everybody can see it and decide 👍 :

[fedonman]

Is this comment needed here?

[GuillermoAbadLopez]

Yeah, good point... I actually have a problem here, which I wanted to ask about:

If you see, Inside the Transpiler we have basically methods duplicated from the Optimizer..., since we removed the handling of multiple circuits at the same time.. One option would be to remove such duplication, but at the same time that duplications serves as a kind of interface to add more functionality, and shows the user which functions can be called from the transpiler directly, so its not super bad...

Of course this makes that the docstrings are duplicated, and that is more problematic, since we wouldn't; want our devs, only updating the Optimizer docstrings, but not the Transpiler's which are more public. So the options we have are:

• Delete the duplication in some way (I think this will complicate how we show public method to the user...)
• Add this silly comment for devs to notice, that they need to change the public documentation always
• Maybe just delete the docstring from the not public methods? So there is no mistake by the devs? And they know to go and search elsewhere, which docstring to change 🤔...

[GuillermoAbadLopez]

I'll think more about it the next days, and ask the group with a clearer head...

But such problem make me notice there is a structural code problem from base with having this duplication, which maybe should be addressed more deeply... But 😬 ...