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

enable resonator as target device component #387

Closed

Conversation

dekelmeirom
Copy link
Contributor

@dekelmeirom dekelmeirom commented Sep 15, 2021

Summary

fixing bug in #171 :
fixing the option to create an experiment with resonator as target device along with qubits (or instead of qubits).

Details and comments

the experiment should include "resonators" in the experiment options to specify the relevant resonators. if there are no qubits as target components, the qubits filed which every experiment has should contain 0 (as int and not as list [0] ).

@dekelmeirom dekelmeirom changed the title [WIP} enable resonator as target device component [WIP] enable resonator as target device component Sep 15, 2021
@dekelmeirom dekelmeirom added this to the Release 0.3 milestone Sep 29, 2021
@dekelmeirom dekelmeirom self-assigned this Sep 29, 2021
@dekelmeirom dekelmeirom changed the title [WIP] enable resonator as target device component enable resonator as target device component Sep 29, 2021
@dekelmeirom dekelmeirom requested a review from yaelbh September 29, 2021 12:39
@jyu00
Copy link
Contributor

jyu00 commented Oct 1, 2021

@dekelmeirom I think the code looks good. However, I don't see any experiment actually puts resonators in the metadata?

@dekelmeirom
Copy link
Contributor Author

@jyu00 you are right, there aren't any such experiments yet.
This is just a fix to enable it (there was an opened bug on this, so I thought that maybe there will be an experiment like resonator spectroscopy added in the future)

@yaelbh
Copy link
Collaborator

yaelbh commented Oct 20, 2021

@jyu What do you suggest to do with this PR for now? Merge or put it on-hold? @eggerdj I think maybe you needed this capability?

@yaelbh yaelbh mentioned this pull request Oct 20, 2021
4 tasks
@wshanks wshanks mentioned this pull request Jan 20, 2022
4 tasks
@wshanks
Copy link
Collaborator

wshanks commented Jan 27, 2022

#583 currently has an alternate implementation that you might want to look at. Currently, it sets experiment_components to the result of calling this method:

https://github.com/Qiskit/qiskit-experiments/blob/6e7c915079530fba45c086a5a244fdf8912d08b1/qiskit_experiments/framework/base_analysis.py#L176-L185

with the expectation that subclasses will override this if they want something other than the qubit components.

One benefit of #583 is that it gives more flexibility for the experiment to set its components. For example, an experiment like #583 that only measures the resonator does not have to include the qubit in its device components as would be the case with this PR (#387), unless the experiment unset physical_qubits but I worry that could have negative side effects.

@chriseclectic chriseclectic removed this from the Release 0.3 milestone Mar 30, 2022
@chriseclectic chriseclectic requested a review from eliarbel March 30, 2022 14:35
@dekelmeirom
Copy link
Contributor Author

I think that #583 fixed the initial problem by enabling the flexibility for the experiment to set its own components.
We can either close this PR without merging, or, I can change the default _get_experiment_components function to enable specifying resonators if the relevant parameters exist (like I did in this PR for the experiment_components function).
the only benefit from it will be that the user will not need to overwrite the _get_experiment_components function in order to have resonators as device components, which might create more compatibility across different experiments in the naming of the resonators components.

Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

The change in _get_experiment_components and the additional test looks good. However my main issue I have for this PR is that the only experiment that needs this change currently, ResonatorSpectroscopy, is not using it in this PR but rather its own solution (overriding _get_experiment_components to return list of Resonator objects from the physical qubits list).

I think for ResonatorSpectroscopy and similar future experiments we should have a way to set the device components to be resonators earlier in the flow, namely at the stage where BaseExperiment._metadata is being called. But here are some questions: 1) adding resonators via overriding _additional_metadata is a not good solution for ResonatorSpectroscopy as we don't need to have both resonators and physical qubits (I think). 2) Using 'resonators' instead of 'physical_qubits' (albeit no nice way for doing this currently), well I'm not sure though how feasible it would be to not have physical_qubits key at all as maybe too many places assume the existence of it.

So bottom-line, I can see 2 options:

  1. Streamline the way experiments specify resonators as components, preferably using some method in BaseExperiments (e.g. set_components) the experiments should override to add (or replace physical_qubits with) resonators entry.
  2. Cancel this PR and continue to use the solution in ResonatorSpectroscopy

@dekelmeirom
Copy link
Contributor Author

as ResonatorSpectroscopy is the only experiment using resonators right now, and it already has its own solution, so I think that option 1 will cause a lot of work which might be unused until (and if) we will have another experiment with component different than qubits (which might need new features we will not implement...).
so I think canceling this PR will be better

@dekelmeirom dekelmeirom closed this Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants