Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coherence Flux #954

Closed
wants to merge 19 commits into from
Closed

Coherence Flux #954

wants to merge 19 commits into from

Conversation

Jacfomg
Copy link
Contributor

@Jacfomg Jacfomg commented Jul 30, 2024

Using #949 as it is does not allow me to create several path during the for loop for the bias an have all the report separate. They will be overwritten as far as I understand. Can you take a look @alecandido or @andrea-pasquale

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.
  • Compatibility with Qibo modules (Please edit this section if the current pull request is not compatible with the following branches).
    • Qibo: master
    • Qibolab: main
    • Qibolab_platforms_qrc: main

@Jacfomg Jacfomg changed the title feat: Coherence Flux Coherence Flux Jul 30, 2024
Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.

Project coverage is 97.46%. Comparing base (9828b46) to head (142255d).
Report is 200 commits behind head on main.

Files with missing lines Patch % Lines
src/qibocal/auto/execute.py 45.45% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
- Coverage   97.55%   97.46%   -0.10%     
==========================================
  Files         118      118              
  Lines        9056     9067      +11     
==========================================
+ Hits         8835     8837       +2     
- Misses        221      230       +9     
Flag Coverage Δ
unittests 97.46% <45.45%> (-0.10%) ⬇️

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

Files with missing lines Coverage Δ
src/qibocal/auto/execute.py 93.49% <45.45%> (-4.72%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Jacfomg for sharing this!
You can find a few suggestions down below.

runcards/coherence_flux.py Outdated Show resolved Hide resolved
Comment on lines +99 to +104
@dataclass
class CoherenceFluxSignalData(Data):
"""Coherence acquisition outputs."""

data: dict[QubitId, npt.NDArray] = field(default_factory=dict)
"""Raw data acquired."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just define a dictionary instead of defining a dataclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would do the job but I like keeping similarities between protocols and sacripts it makes things simpler IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote goes for skipping the 1-parameter class (there is not even any method associated, so it is truly a wrapper, not even used for the type...)


# D2
params_qubit = {
"w_max": 5.552552628640306 * 1e9, # FIXME: this is not the qubit frequency
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but at the same time what we usually have in the runcard is the maximum frequency, right?

Copy link
Contributor Author

@Jacfomg Jacfomg Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we usually have in the runcard is something wrong then 🤷‍♂️

runcards/coherence_flux.py Outdated Show resolved Hide resolved
runcards/coherence_flux.py Outdated Show resolved Hide resolved
runcards/coherence_flux.py Outdated Show resolved Hide resolved
@andrea-pasquale
Copy link
Contributor

andrea-pasquale commented Jul 30, 2024

Using #949 as it is does not allow me to create several path during the for loop for the bias an have all the report separate.

I think that the correct of way of using it would be for you to define an executor for each flux point.
Something like:

for bias in biases:
    with Executor.open(
    "myexec",
    path="test_rx_calibration",
    platform="dummy",
    targets=[target],
    update=True,
    force=True,
) as e:
    ....

Would this work for you?

@Jacfomg
Copy link
Contributor Author

Jacfomg commented Jul 30, 2024

Using #949 as it is does not allow me to create several path during the for loop for the bias an have all the report separate.

I think that the correct of way of using it would be for you to define an executor for each flux point. Something like:

for bias in biases:
    with Executor.open(
    "myexec",
    path="test_rx_calibration",
    platform="dummy",
    targets=[target],
    update=True,
    force=True,
) as e:
    ....

Would this work for you?

That would save the protocol outputs but then I would like to save the data used on the final plotting on another folder

@andrea-pasquale
Copy link
Contributor

Using #949 as it is does not allow me to create several path during the for loop for the bias an have all the report separate.

I think that the correct of way of using it would be for you to define an executor for each flux point. Something like:

for bias in biases:
    with Executor.open(
    "myexec",
    path="test_rx_calibration",
    platform="dummy",
    targets=[target],
    update=True,
    force=True,
) as e:
    ....

Would this work for you?

That would save the protocol outputs but then I would like to save the data used on the final plotting on another folder

Ok then you could pass through CLI the folder for the final plot (for example).
Or if for the bias point you do something like "folder_{bias}" you could just have "folder" for the final plot.

@alecandido
Copy link
Member

Or if for the bias point you do something like "folder_{bias}" you could just have "folder" for the final plot.

Indeed:

  • each Executor has one folder, where all its output is stored, for all the protocols executed
  • the report function just takes a path to an Executor folder, and generates a report with all the protocols in there

So, you have two options:

  1. use many executors (or update many times its path attribute) and generate many output folders, out of which generating many reports (with one call to the report() function for each of them)
  2. use a single executor, but change id to the various protocols, to save all of them in separate subfolders of the same root one, and then generate a single report (with one report() invocation) containing all the protocols

Intermediate options are available as well, since you can batch them the way you prefer.

Which one is the most convenient depends on the use case. For a limited range of values, which will be used altogether (e.g. comparing them), I'd go for option 2.. If instead you have many values, I'd organize them according to some hierarchy (which in principle could be done even with 2., but you should filter them manually during reports' generation, which is not available in a user-friendly way).

@Jacfomg
Copy link
Contributor Author

Jacfomg commented Jul 31, 2024

Or if for the bias point you do something like "folder_{bias}" you could just have "folder" for the final plot.

Indeed:

  • each Executor has one folder, where all its output is stored, for all the protocols executed
  • the report function just takes a path to an Executor folder, and generates a report with all the protocols in there

So, you have two options:

  1. use many executors (or update many times its path attribute) and generate many output folders, out of which generating many reports (with one call to the report() function for each of them)
  2. use a single executor, but change id to the various protocols, to save all of them in separate subfolders of the same root one, and then generate a single report (with one report() invocation) containing all the protocols

Intermediate options are available as well, since you can batch them the way you prefer.

Which one is the most convenient depends on the use case. For a limited range of values, which will be used altogether (e.g. comparing them), I'd go for option 2.. If instead you have many values, I'd organize them according to some hierarchy (which in principle could be done even with 2., but you should filter them manually during reports' generation, which is not available in a user-friendly way).

I will go with the 1st with the small fix of defining a first unused executed to get some qubit parameters from the platform instead of defining them at every executor

@alecandido
Copy link
Member

I will go with the 1st with the small fix of defining a first unused executed to get some qubit parameters from the platform instead of defining them at every executor

You can also instantiate the platform without an executor :)

@alecandido
Copy link
Member

You can also instantiate the platform without an executor :)

And, in that case, you will receive a platform with all targets, so you can just query those you want, without instantiating over and over for each target (btw, this is also true for the executor: it executes the routines for a specific target, but its platform always contains all possible targets).

@Jacfomg Jacfomg marked this pull request as ready for review July 31, 2024 08:41
@Jacfomg Jacfomg requested a review from alecandido July 31, 2024 08:41
Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick look at the first script

runcards/coherence_flux.py Outdated Show resolved Hide resolved
runcards/coherence_flux.py Outdated Show resolved Hide resolved
runcards/coherence_flux.py Outdated Show resolved Hide resolved
Comment on lines +99 to +104
@dataclass
class CoherenceFluxSignalData(Data):
"""Coherence acquisition outputs."""

data: dict[QubitId, npt.NDArray] = field(default_factory=dict)
"""Raw data acquired."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote goes for skipping the 1-parameter class (there is not even any method associated, so it is truly a wrapper, not even used for the type...)

@Jacfomg
Copy link
Contributor Author

Jacfomg commented Jul 31, 2024

Quick look at the first script

Regarding the CoherenceFluxSignalData I'm using it since I felt using the data.register_qubit is convenient

@Jacfomg Jacfomg mentioned this pull request Jul 31, 2024
8 tasks
@andrea-pasquale andrea-pasquale added the experiment Related to an individual experiment (/protocol/routine/...) label Aug 12, 2024
Base automatically changed from exec-ctx-manager to main August 13, 2024 09:43
@andrea-pasquale andrea-pasquale added the do not merge Soft warning to avoid merging a PR for reason provided in the PR label Aug 14, 2024
@Edoardo-Pedicillo Edoardo-Pedicillo mentioned this pull request Sep 11, 2024
8 tasks
@Jacfomg
Copy link
Contributor Author

Jacfomg commented Sep 20, 2024

because of #981 I can close it.

@Jacfomg Jacfomg closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Soft warning to avoid merging a PR for reason provided in the PR experiment Related to an individual experiment (/protocol/routine/...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants