Improvements to `ActionBuilder` #271

andrea-pasquale · 2023-03-08T09:32:19Z

This PR adds some improvements to the ActionBuilder including

@andrea-pasquale Move the function outside of the ActionBuilder (from Stavros comment non interactive gate set characterization + action builder #239 (comment)):
from Small changes for niGSC part #257.

I'm keeping this PR as draft for the moment since I also want to fix #267 here.

EDIT
Other things fixed by this PR:

Fix Improve switcher between calibration and RB #267
Fix table in report for RB routines
Enable qq-live for RB routines

Checklist:

Reviewers confirm new code works as expected.
Tests are passing.
Coverage does not decrease.
Documentation is updated.

codecov · 2023-03-08T09:32:37Z

Codecov Report

Merging #271 (1cdd8dd) into main (4e329b3) will increase coverage by 0.17%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
+ Coverage   36.57%   36.74%   +0.17%     
==========================================
  Files          44       44              
  Lines        3027     3029       +2     
==========================================
+ Hits         1107     1113       +6     
+ Misses       1920     1916       -4

Flag	Coverage Δ
unittests	`36.74% <0.00%> (+0.17%)`	⬆️

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

Impacted Files	Coverage Δ
src/qibocal/web/app.py	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

andrea-pasquale · 2023-04-07T09:26:07Z

@wilkensJ when you have time could you please check this PR?

wilkensJ

So in general multiple things will happen in the next PR, which @vodovozovaliza and me are working on right now, this will also influence this PR:

Use a global function for aggregate action function, because many module are sharing the same structure and functions, we will put them in the folder basics
The amount of fitting parameters will be flexible
The plotly figure generation will be a bit different

src/qibocal/calibrations/niGSC/XIdrb.py

src/qibocal/calibrations/niGSC/basics/plot.py

andrea-pasquale · 2023-04-11T08:13:49Z

So in general multiple things will happen in the next PR, which @vodovozovaliza and me are working on right now, this will also influence this PR:

Use a global function for aggregate action function, because many module are sharing the same structure and functions, we will put them in the folder basics

The amount of fitting parameters will be flexible

The plotly figure generation will be a bit different

Thanks for the feedback @wilkensJ.
I understand that there is a PR that will change multiple things.
I think that this PR should be merged before given that it fixes some annoying issues including #267 and also some other minor problems to improve the compatibility between the rb routines and the rest of the code as you can see in the top comments.
If the fixes proposed in this PR work I suggest to proceed with the merge than we can have a review of the other PR related to the niGSC.

wilkensJ · 2023-04-11T14:47:19Z

I think in the niGSC setting the fitting parameter table still has to be improved, for example for three qubits the simulfilteredrb module creates a not very easy to read report:

Actually, this part did not fit on my display, not even the table itself fit on my display (to be fair it is a particularly small display). Also, the parameters are hard to connect with the right fit (the user has to actively count the figures in order to connect them).

After the adjustments I (partially) left in the review comments (not the change from always q0 to the actual irrep)

Of course also this needs some adjustments (the width of the table is not ideal, I put it to 50%, the spacing between the plots is too big, this is fixed in #293, also there will be titles for the legend, thanks to Liza, also the naming of the table is not idea now)
But this solution does not require so much space (there even could be a third fitting parameter added without making the table even longer), it is easier to read and identify the fits with the parameters.

wilkensJ · 2023-04-11T17:59:59Z

As I see it, one of the main features added to the niGSC is the fitting parameter table. I think it is great. Also, moving the method of generating an output folder outside of the ActionBuilder class and using it for the Experiment class is great.

Regarding the adjustments to the niGSC branch, I appreciate your ideas and will definitely take them into consideration, however, I think that it would be more sustainable to align them with the current state or make slight changes from the main branch as Liza and I have some ideas on where the ‘niGSC’ part will go and are currently working on a new PR related to RB modules.

andrea-pasquale · 2023-04-12T07:40:01Z

Regarding the adjustments to the niGSC branch, I appreciate your ideas and will definitely take them into consideration, however, I think that it would be more sustainable to align them with the current state or make slight changes from the main branch as Liza and I have some ideas on where the ‘niGSC’ part will go and are currently working on a new PR related to RB modules.

If you and @vodovozovaliza are going to modify a few things, to me it is fine.
Regarding this PR, I would like to keep the things which are not included in #293, especially the fix for #267. Let me know what you don't like and I will remove it from this PR.

wilkensJ · 2023-04-26T10:39:03Z

Hello Andrea,

I would like to ask if you had a chance to look at the comments in the PR as they are still marked as pending. I am sorry if any of the comments lead to misunderstanding, if something is not helpful or confusing in any way, I will be glad to clarify it. Please let us know what we (@vodovozovaliza and me) can do such that we can merge this PR, sorry again for any confusion.

andrea-pasquale · 2023-04-27T10:55:32Z

Hi @wilkensJ,
following today's discussion some of the things in this PR may become outdated.
With this PR I only attempted to make the RB part in qibocal more compatible with the rest of the code which is currently in main. I think that this should more or less what we will happen soon when the RB is coded using the autocalibration layout.
I know that several things may not be the best for now since we are still underdevelopment.
If you think that there are some critical changes here that you don't like feel free to open a PR on top of this one to remove those changes and add me as a reviewer. Otherwise we could either merge or close directly this PR.
Let me know what you prefer to do.

vodovozovaliza · 2023-04-28T15:29:03Z

Hi @wilkensJ,
following today's discussion some of the things in this PR may become outdated.
With this PR I only attempted to make the RB part in qibocal more compatible with the rest of the code which is currently in main. I think that this should more or less what we will happen soon when the RB is coded using the autocalibration layout.
I know that several things may not be the best for now since we are still underdevelopment.
If you think that there are some critical changes here that you don't like feel free to open a PR on top of this one to remove those changes and add me as a reviewer. Otherwise we could either merge or close directly this PR.
Let me know what you prefer to do.

Hello @andrea-pasquale, thank you very much for the improvements to the ActionBuilder. There are a few modifications that are important for the current protocols, so @wilkensJ and I think it would be best to create a PR on top of this one to combine the necessary changes and add them to main. I was wondering if it would be possible for you to merge the main branch into simplify_builder so that we can create a PR on top of it.

for more information, see https://pre-commit.ci

Co-authored-by: Andrea Pasquale <[email protected]>

NIGSC changes

andrea-pasquale · 2023-05-03T12:06:16Z

@wilkensJ @vodovozovaliza after the fixes from #318 this should be ready to merge, right?

vodovozovaliza

Thank you very much @andrea-pasquale, everything looks good to me here

src/qibocal/calibrations/niGSC/basics/plot.py

src/qibocal/calibrations/niGSC/basics/utils.py

src/qibocal/calibrations/niGSC/simulfilteredrb.py

src/qibocal/calibrations/niGSC/standardrb.py

src/qibocal/cli/builders.py

wilkensJ · 2023-04-10T13:37:21Z

src/qibocal/cli/utils.py

+    """Generation of qq output folder
+    Args:


Here has to be an empty line, right?

src/qibocal/calibrations/niGSC/XIdrb.py

src/qibocal/calibrations/niGSC/basics/plot.py

wilkensJ · 2023-05-04T08:31:07Z

Hello @andrea-pasquale, I hope I did not mess up my review, I resolved the comment which are outdated, was that okay? Or is it making things complicated? There are two comments I did not resolve. It would be great if you could implement them.
Also, if it is not too much work and maybe unnecessary because I oversaw something, could the plotting per qubit be disabled our handled differently? Maybe this is already possible, I am sorry if this is solved already, in this PR it is still not.

For example if this runcard is run:

backend: numpy

qubits: [0,1,2]

actions:
  simulfilteredrb:
    nqubits: 3
    depths: [1,3,5,7,10]
    runs: 2
    nshots: 1024
    noise_model: PauliErrorOnAll
    noise_params: [0.01, 0.01, 0.01]

There will be three reports of the same plots.
Again, sorry if that is fixed already.

src/qibocal/calibrations/niGSC/simulfilteredrb.py

src/qibocal/calibrations/niGSC/basics/fitting.py

src/qibocal/cli/utils.py

Co-authored-by: vodovozovaliza <[email protected]>

andrea-pasquale · 2023-05-08T21:44:17Z

Hello @andrea-pasquale, I hope I did not mess up my review, I resolved the comment which are outdated, was that okay? Or is it making things complicated? There are two comments I did not resolve. It would be great if you could implement them. Also, if it is not too much work and maybe unnecessary because I oversaw something, could the plotting per qubit be disabled our handled differently? Maybe this is already possible, I am sorry if this is solved already, in this PR it is still not.

For example if this runcard is run:
backend: numpy

qubits: [0,1,2]

actions:
  simulfilteredrb:
    nqubits: 3
    depths: [1,3,5,7,10]
    runs: 2
    nshots: 1024
    noise_model: PauliErrorOnAll
    noise_params: [0.01, 0.01, 0.01]
There will be three reports of the same plots. Again, sorry if that is fixed already.

Thanks for the review @wilkensJ and @vodovozovaliza.
This feature is already implemented within the autocalibration layout and it will require some work to add it in the old layout. Since all routines will eventually move to the new layout I think that we could avoid to implement this feature here. Let me know if you agree.

vodovozovaliza · 2023-05-10T10:38:01Z

The examples in the documentation will be outdated once the branch is merged. Is it okay if I create a separate PR for them when we merge this PR?

andrea-pasquale added 3 commits March 7, 2023 14:36

Moving and simplyfing methods

6306c30

Fix tests

5a3bf03

Add forgotten file

cf33c76

andrea-pasquale added the enhancement New feature or request label Mar 8, 2023

andrea-pasquale added this to the Qibocal 0.0.2 milestone Mar 8, 2023

andrea-pasquale self-assigned this Mar 8, 2023

andrea-pasquale added 4 commits March 8, 2023 15:53

Remove try-except block

37f683d

Fix plotting functions

e8f3358

Minor fixes and enable qq-live for RB

00e5392

Fix tests

6618fc8

andrea-pasquale marked this pull request as ready for review March 10, 2023 08:58

andrea-pasquale requested review from scarrazza, stavros11 and wilkensJ March 10, 2023 08:59

wilkensJ reviewed Apr 10, 2023

View reviewed changes

Merge branch 'main' into simplify_builder

cd71901

vodovozovaliza added 3 commits April 28, 2023 15:43

af29b7b

Minor changes

0b075d5

Create fitting_report tables

52d3b13

andrea-pasquale and others added 2 commits April 28, 2023 19:40

Merge branch 'main' into simplify_builder

8b1402e

Fix tests

b85fdb6

wilkensJ and others added 10 commits May 2, 2023 21:05

Improving nigsc plotting

3c3823a

[pre-commit.ci] auto fixes from pre-commit.com hooks

59a93b5

for more information, see https://pre-commit.ci

improve plot | fix coverage

7588c4d

[pre-commit.ci] auto fixes from pre-commit.com hooks

6c5612c

for more information, see https://pre-commit.ci

Update src/qibocal/calibrations/niGSC/basics/experiment.py

b21a5c3

Co-authored-by: Andrea Pasquale <[email protected]>

Update src/qibocal/calibrations/niGSC/basics/experiment.py

02df578

Co-authored-by: Andrea Pasquale <[email protected]>

Update src/qibocal/calibrations/niGSC/basics/experiment.py

1a2cf1c

Co-authored-by: Andrea Pasquale <[email protected]>

Update src/qibocal/calibrations/niGSC/basics/experiment.py

f4416ec

Co-authored-by: Andrea Pasquale <[email protected]>

reverse table format

12e587b

Merge pull request #318 from qiboteam/nigsc_changes

4277bad

NIGSC changes

andrea-pasquale requested review from vodovozovaliza and wilkensJ May 3, 2023 12:08

vodovozovaliza approved these changes May 3, 2023

View reviewed changes

wilkensJ reviewed May 4, 2023

View reviewed changes

vodovozovaliza suggested changes May 5, 2023

View reviewed changes

src/qibocal/calibrations/niGSC/simulfilteredrb.py Outdated Show resolved Hide resolved

src/qibocal/calibrations/niGSC/basics/fitting.py Show resolved Hide resolved

src/qibocal/cli/utils.py Show resolved Hide resolved

Apply Liza' suggestions

cc265c8

Co-authored-by: vodovozovaliza <[email protected]>

andrea-pasquale requested review from wilkensJ and vodovozovaliza May 8, 2023 21:45

vodovozovaliza approved these changes May 10, 2023

View reviewed changes

Merge branch 'main' into simplify_builder

1cdd8dd

wilkensJ approved these changes May 15, 2023

View reviewed changes

andrea-pasquale added this pull request to the merge queue May 16, 2023

Merged via the queue into main with commit 1527449 May 16, 2023

andrea-pasquale deleted the simplify_builder branch May 16, 2023 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to `ActionBuilder` #271

Improvements to `ActionBuilder` #271

andrea-pasquale commented Mar 8, 2023 •

edited

Loading

codecov bot commented Mar 8, 2023 •

edited

Loading

andrea-pasquale commented Apr 7, 2023

wilkensJ left a comment

andrea-pasquale commented Apr 11, 2023 •

edited

Loading

wilkensJ commented Apr 11, 2023

wilkensJ commented Apr 11, 2023

andrea-pasquale commented Apr 12, 2023

wilkensJ commented Apr 26, 2023

andrea-pasquale commented Apr 27, 2023

vodovozovaliza commented Apr 28, 2023

andrea-pasquale commented May 3, 2023

vodovozovaliza left a comment

wilkensJ Apr 10, 2023

wilkensJ commented May 4, 2023

andrea-pasquale commented May 8, 2023

vodovozovaliza commented May 10, 2023

Improvements to ActionBuilder #271

Improvements to ActionBuilder #271

Conversation

andrea-pasquale commented Mar 8, 2023 • edited Loading

codecov bot commented Mar 8, 2023 • edited Loading

Codecov Report

andrea-pasquale commented Apr 7, 2023

wilkensJ left a comment

Choose a reason for hiding this comment

andrea-pasquale commented Apr 11, 2023 • edited Loading

wilkensJ commented Apr 11, 2023

wilkensJ commented Apr 11, 2023

andrea-pasquale commented Apr 12, 2023

wilkensJ commented Apr 26, 2023

andrea-pasquale commented Apr 27, 2023

vodovozovaliza commented Apr 28, 2023

andrea-pasquale commented May 3, 2023

vodovozovaliza left a comment

Choose a reason for hiding this comment

wilkensJ Apr 10, 2023

Choose a reason for hiding this comment

wilkensJ commented May 4, 2023

andrea-pasquale commented May 8, 2023

vodovozovaliza commented May 10, 2023

Improvements to `ActionBuilder` #271

Improvements to `ActionBuilder` #271

andrea-pasquale commented Mar 8, 2023 •

edited

Loading

codecov bot commented Mar 8, 2023 •

edited

Loading

andrea-pasquale commented Apr 11, 2023 •

edited

Loading