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

kernels #582

Merged
merged 48 commits into from
Feb 2, 2024
Merged

kernels #582

merged 48 commits into from
Feb 2, 2024

Conversation

Jacfomg
Copy link
Contributor

@Jacfomg Jacfomg commented Oct 25, 2023

Prototype routine to test the kernel computation for integration using zhinst functions.
Also, results.npz and result.json will be generated, which I didn't do correctly and all routines crash.

Tests will fail until we merge qiboteam/qibolab#752 to get kernels in the dummy.

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 mentioned this pull request Oct 25, 2023
8 tasks
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 implementing this.
I still need to run the protocol.
The modifications in the API look good to me.

Comment on lines 206 to 209
if "data" not in self.__dict__:
self.data: Optional[
dict[Union[tuple[QubitId, int], QubitId], npt.NDArray]
] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this directly in the __init__?
With default value None

Copy link
Contributor Author

@Jacfomg Jacfomg Nov 2, 2023

Choose a reason for hiding this comment

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

No, because then it was complaning that I was giving after other non default arguments. I mean, we should do something else if we want it to get it out of there.

Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, try to implement the same approach of the Parameters class, to be consistent with the rest of the code.

@classmethod
def load(cls, input_parameters):
"""Load parameters from runcard.
Possibly looking into previous steps outputs.
Parameters defined in Parameters class are removed from `parameters`
before `cls` is created.
Then `nshots` and `relaxation_time` are assigned to cls.
.. todo::
move the implementation to History, since it is required to resolve
the linked outputs
"""
default_parent_parameters = deepcopy(DEFAULT_PARENT_PARAMETERS)
parameters = deepcopy(input_parameters)
for parameter, value in default_parent_parameters.items():
default_parent_parameters[parameter] = parameters.pop(parameter, value)
instantiated_class = cls(**parameters)
for parameter, value in default_parent_parameters.items():
setattr(instantiated_class, parameter, value)
return instantiated_class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this what @GabrielePalazzo did some time ago ? It was not very clear to me what it was doing could you lend me a hand ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also try something easier like

nboot = parameters.pop("nboot", 0)
par = cls(**parameters)
par.nboot = nboot
return par

src/qibocal/auto/operation.py Show resolved Hide resolved
@Edoardo-Pedicillo
Copy link
Contributor

Is this PR in draft or under review?

@Jacfomg
Copy link
Contributor Author

Jacfomg commented Nov 2, 2023

Well, it was draft because the results were not the expected, I'm waiting for comments from Zurich. But the changes to the API can be reviewed and maybe put them on another PR so they get merged faster.

Comment on lines 206 to 209
if "data" not in self.__dict__:
self.data: Optional[
dict[Union[tuple[QubitId, int], QubitId], npt.NDArray]
] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, try to implement the same approach of the Parameters class, to be consistent with the rest of the code.

@classmethod
def load(cls, input_parameters):
"""Load parameters from runcard.
Possibly looking into previous steps outputs.
Parameters defined in Parameters class are removed from `parameters`
before `cls` is created.
Then `nshots` and `relaxation_time` are assigned to cls.
.. todo::
move the implementation to History, since it is required to resolve
the linked outputs
"""
default_parent_parameters = deepcopy(DEFAULT_PARENT_PARAMETERS)
parameters = deepcopy(input_parameters)
for parameter, value in default_parent_parameters.items():
default_parent_parameters[parameter] = parameters.pop(parameter, value)
instantiated_class = cls(**parameters)
for parameter, value in default_parent_parameters.items():
setattr(instantiated_class, parameter, value)
return instantiated_class

…brate_state_discrimination.py

Co-authored-by: Edoardo Pedicillo <[email protected]>
@Jacfomg
Copy link
Contributor Author

Jacfomg commented Nov 24, 2023

The calculation of the kernel calculation was moved https://docs.zhinst.com/labone_q_user_manual/release_notes/

@Jacfomg
Copy link
Contributor Author

Jacfomg commented Nov 29, 2023

It needs laboneq 2.20.0 version that will get merged soon. And testing on hardware.

@andrea-pasquale andrea-pasquale added the qibolab release required PR requires qibolab feature not available in the current poetry.lock label Nov 29, 2023
@Jacfomg Jacfomg marked this pull request as ready for review December 4, 2023 09:01
@Jacfomg Jacfomg changed the base branch from main to rmstart January 16, 2024 08:36
@andrea-pasquale andrea-pasquale mentioned this pull request Jan 16, 2024
8 tasks
Base automatically changed from rmstart to main January 27, 2024 09:09
@andrea-pasquale
Copy link
Contributor

Thanks @GabrielePalazzo for the review. Your comments have been addressed.

@andrea-pasquale andrea-pasquale added calibration enhancement New feature or request labels Feb 2, 2024
@andrea-pasquale andrea-pasquale added this pull request to the merge queue Feb 2, 2024
Merged via the queue into main with commit 438bc57 Feb 2, 2024
21 checks passed
@andrea-pasquale andrea-pasquale deleted the kernels branch February 2, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calibration enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants