NIGSC changes #318

vodovozovaliza · 2023-04-29T10:35:24Z

Checklist:

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

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

andrea-pasquale

Thanks @vodovozovaliza for implementing this.
I started with a quick a review even if this is still in draft.
I agree with the changes proposed by this PR.
You can find below a few comments.

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

andrea-pasquale · 2023-05-01T05:40:22Z

tests/test_niGSC_circuitfactory.py

@@ -41,7 +39,7 @@ def test_abstract_factory():
    cfactory = CircuitFactory(1, [1, 2] * 3, qubits=[0])
    with pytest.raises(NotImplementedError):
        for circuit in cfactory:
-            print(circuit.draw())
+            circuit.draw()


This could probably be removed I think

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

codecov · 2023-05-01T06:49:56Z

Codecov Report

Merging #318 (12e587b) into simplify_builder (8b1402e) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@                Coverage Diff                @@
##           simplify_builder     #318   +/-   ##
=================================================
  Coverage             38.04%   38.04%           
=================================================
  Files                    42       42           
  Lines                  2802     2802           
=================================================
  Hits                   1066     1066           
  Misses                 1736     1736

Flag	Coverage Δ
unittests	`38.04% <ø> (ø)`

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

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

vodovozovaliza · 2023-05-02T10:21:22Z

@andrea-pasquale @wilkensJ this is ready for review

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

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

wilkensJ · 2023-05-01T06:57:39Z

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

+        circuit_init_kwargs = deepcopy(circuit.init_kwargs)
+        del circuit_init_kwargs["nqubits"]
+        bigcircuit = Circuit(self.nqubits, **circuit_init_kwargs)
+        bigcircuit.add(circuit.on_qubits(*self.qubits))


Maybe this is clearer? I think that I is just a different taste, right?

circuit_init_kwargs = deepcopy(circuit.init_kwargs) circuit_init_kwargs['nqubits'] = self.nqubits bigcircuit = models.Circuit(**circuit.init_kwargs) bigcircuit.add(circuit.on_qubits(*self.qubits))

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

wilkensJ · 2023-05-02T18:25:45Z

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

        )
+    )
    fig_traces.append(
        go.Scatter(
            x=x_fit,


Is it possible to put the fitting parameters in the label of the fit? If there are more than 2 plots I think the graphic is easier to read and to interact with.

wilkensJ · 2023-05-02T19:07:27Z

src/qibocal/calibrations/niGSC/XIdrb.py

@@ -137,13 +134,14 @@ def post_processing_sequential(experiment: Experiment):
 # After the row by row execution of tasks comes the aggregational task. Something like calculation
 # of means, deviations, fitting data, tasks where the whole data as to be looked at, and not just
 # one instance of circuit + other information.
-def get_aggregational_data(experiment: Experiment) -> pd.DataFrame:
+def get_aggregational_data(experiment: Experiment, ndecays: int = 2) -> pd.DataFrame:


How can the number of decays be changed through a runcard?

This parameter is not used in the runcards for now, but can be used to redo the fitting of the same experiment

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

andrea-pasquale

Thanks @vodovozovaliza.
You can find below a few comments.
I think that we should be able to merge this PR today.

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

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

andrea-pasquale · 2023-05-03T06:21:46Z

src/qibocal/web/static/styles.css

This modifications also affect the report generated by the low level routines.
Moving the table to the left is not ideal for the routine which include more than one plot.
I believe this is out of the scope of this PR, we could discuss about the report generation in a separate PR/issue.

I tried it with the routines from runcard/actions_qq.yml (a simulation) and for me it looked fine, the only change here is that it is not centered anymore and there is a bit of more whitespace between the columns.
It would be nice if you could try to generate reports with the new css settings and if something is not readable anymore I totally agree to not implement that.
Otherwise, when the table has only a 30% width our fitting parameters are in two lines and that does influence how they are read, also, it takes a lot of space, so if there are more than lets say 5 lines the breaking does make it harder to read. The 50% was as a bit arbitrary, maybe 40% would also do that, or the margin to the left can be more, that was also more of a try and error, I am very open for new ideas here.

I totally agree though that the centered looks more nice:

But it does not do any harm if it is not centered anymore:

With our report:

vs

I have a problem only with the position of the table.
If you run for example t1 with dummy it looks like this:

which personally I don't like. As you said centered looks better.
I also understand that the current rendering may not best for the RB protocols.
For single plot you could try to stretch out the plot like we do for the T1 above.
For multiple plots I think that the table in the center should look ok.
But, again we could fix this in a separate PR and we should also ask the opinion of other people.

I agree! This looks weird. I like the idea of putting it in another PR :)

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

wilkensJ · 2023-05-03T11:25:38Z

For me this would be fine to merge with the simply_builder now :)

andrea-pasquale

LGTM, thanks @vodovozovaliza.

vodovozovaliza added 3 commits April 28, 2023 15:43

af29b7b

Minor changes

0b075d5

Create fitting_report tables

52d3b13

vodovozovaliza requested review from andrea-pasquale and wilkensJ April 29, 2023 10:35

vodovozovaliza and others added 5 commits April 29, 2023 14:41

Fix tests

b85fdb6

Merge branch 'simplify_builder' into nigsc_changes

6ed1edb

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

17bd410

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

nigsc

ea94088

Fix tests

499a2af

andrea-pasquale reviewed May 1, 2023

View reviewed changes

Add suggestions

d926f1d

vodovozovaliza and others added 4 commits May 1, 2023 11:26

Fix warnings.

9391972

Minor changes

f6bcd1c

main runcard | embed circuit better

b028c51

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

5f4952d

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

vodovozovaliza marked this pull request as ready for review May 1, 2023 09:35

wilkensJ and others added 2 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

wilkensJ reviewed May 2, 2023

View reviewed changes

vodovozovaliza and others added 2 commits May 3, 2023 10:18

improve plot | fix coverage

7588c4d

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

6c5612c

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

andrea-pasquale reviewed May 3, 2023

View reviewed changes

vodovozovaliza and others added 5 commits May 3, 2023 09:31

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

wilkensJ approved these changes May 3, 2023

View reviewed changes

andrea-pasquale approved these changes May 3, 2023

View reviewed changes

andrea-pasquale merged commit 4277bad into simplify_builder May 3, 2023

andrea-pasquale deleted the nigsc_changes branch May 3, 2023 12:05

andrea-pasquale mentioned this pull request May 3, 2023

Improvements to ActionBuilder #271

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIGSC changes #318

NIGSC changes #318

vodovozovaliza commented Apr 29, 2023 •

edited by renatomello

Loading

andrea-pasquale left a comment

andrea-pasquale May 1, 2023

codecov bot commented May 1, 2023 •

edited

Loading

vodovozovaliza commented May 2, 2023

wilkensJ May 1, 2023

wilkensJ May 2, 2023

wilkensJ May 2, 2023 •

edited

Loading

vodovozovaliza May 2, 2023

andrea-pasquale left a comment

andrea-pasquale May 3, 2023

wilkensJ May 3, 2023 •

edited

Loading

andrea-pasquale May 3, 2023

wilkensJ May 3, 2023

wilkensJ commented May 3, 2023

andrea-pasquale left a comment

NIGSC changes #318

NIGSC changes #318

Conversation

vodovozovaliza commented Apr 29, 2023 • edited by renatomello Loading

andrea-pasquale left a comment

Choose a reason for hiding this comment

andrea-pasquale May 1, 2023

Choose a reason for hiding this comment

codecov bot commented May 1, 2023 • edited Loading

Codecov Report

vodovozovaliza commented May 2, 2023

wilkensJ May 1, 2023

Choose a reason for hiding this comment

wilkensJ May 2, 2023

Choose a reason for hiding this comment

wilkensJ May 2, 2023 • edited Loading

Choose a reason for hiding this comment

vodovozovaliza May 2, 2023

Choose a reason for hiding this comment

andrea-pasquale left a comment

Choose a reason for hiding this comment

andrea-pasquale May 3, 2023

Choose a reason for hiding this comment

wilkensJ May 3, 2023 • edited Loading

Choose a reason for hiding this comment

andrea-pasquale May 3, 2023

Choose a reason for hiding this comment

wilkensJ May 3, 2023

Choose a reason for hiding this comment

wilkensJ commented May 3, 2023

andrea-pasquale left a comment

Choose a reason for hiding this comment

vodovozovaliza commented Apr 29, 2023 •

edited by renatomello

Loading

codecov bot commented May 1, 2023 •

edited

Loading

wilkensJ May 2, 2023 •

edited

Loading

wilkensJ May 3, 2023 •

edited

Loading