-
Notifications
You must be signed in to change notification settings - Fork 42
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
Converted T1 #277
Converted T1 #277
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Omrie, the refactoring looks great! I have been extra picky in my additional comments because I think it will have a benefit to how we write code and refactor nodes in the long run. There is also an important functional change that needs to be made to the behaviour of multiplexing.
multiplexed: bool = False | ||
|
||
node = QualibrationNode(name="05_T1", parameters=Parameters()) | ||
node = QualibrationNode(name="05_T1", parameters=Parameters(qubits = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indent it as follows:
QualibrationNode(
name="05_T1",
parameters=Parameters(
qubits = None,
num_averages = 100,
min_wait_time_in_ns = 16,
max_wait_time_in_ns = 100000,
wait_time_step_in_ns = 600,
flux_point_joint_or_independent_or_arbitrary = "independent",
reset_type = "thermal",
use_state_discrimination = False,
simulate = False,
simulation_duration_ns = 2500,
timeout = 100,
load_data_id = None,
multiplexed = False
)
)
This kind of nesting makes it clearer that name
and parameters
are attributes of the QualibrationNode
, and that the parameters belong to node.parameters
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
flux_point_joint_or_independent_or_arbitrary: Literal["joint", "independent", "arbitrary"] = "independent" | ||
reset_type: Literal["active", "thermal"] = "thermal" | ||
use_state_discrimination: bool = False | ||
simulate: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you have re-defined many of the parameters which will be inherited from the parent classes.
Please remove those which are duplicated, i.e.,
simulate: bool = False
simulation_duration_ns: int = 2500
timeout: int = 100
load_data_id: Optional[int] = None
multiplexed: bool = False
use_waveform_report: bool = False
flux_point_joint_or_independent_or_arbitrary: Literal["joint", "independent", "arbitrary"] = "independent"
And any others that I missed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I now see what you did there. It is nice.
config = machine.generate_config() | ||
# Open Communication with the QOP | ||
if node.parameters.load_data_id is None: | ||
qmm = machine.connect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a new line before the start of the # %% {QUA_program}
section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
qubits = [machine.qubits[q] for q in node.parameters.qubits] | ||
num_qubits = len(qubits) | ||
|
||
|
||
# %% {QUA_program} | ||
n_avg = node.parameters.num_averages # The number of averages | ||
# Dephasing time sweep (in clock cycles = 4ns) - minimum is 4 clock cycles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we create variables which will be used in the program:
idle_times
detunings
arb_flux_bias_offset
Glancing at how these variables are created doesn't really give me any useful information about what what they are doing and how they will be used. So, I think it should be put into a separate module.
The module I think it fits best in would be quam_libs/experiments/T1/parameters.py
, specifically as a method to the Parameters
class. But, due to this PR in QUAlibrate, methods of Parameters
classes get removed. Instead, please make it into functions that live in the same module.
The outcome in the main node is that these values should be acquired in the following way:
from quam_libs.experiments.T1.parameters import Parameters
...
idle_times = get_idle_times_in_clock_cycles(node.parameters)
detunings = ... # this doesn't actually get used. Ask Tom if it's a bug that needs fixing, or should be removed
# maybe think of a better name for this? It feels like 2 or 3 of the "flux", "bias", "offset" and "arb" are redundant
arb_flux_bias_offset = get_arb_flux_bias_offset_for_each_qubit(node.parameters, qubits)
For reference, please see the refactored Ramsey node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Deleted detuning (checked with Tom). arb_flux_bias_offset changed to arb_flux_offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note on the behaviour of the program- the behaviour when setting multiplexed = True
is actually wrong. What we want is behaviour that is multiplexed and aligned.
Given how it's currently written, imagine that multiplexed=True
, then imagine what would happen in any of the following scenarios:
- active reset takes longer to "succeed" for q1 than q2 (if using active reset)
- thermal reset takes longer for q1 than q2 due to different T1s (if using thermal reset)
- the pi-pulse times are different between q1 and q2
- the readout between q1 and q2 have different durations
If it isn't clear straight away, you should visualize this using the waveform report.
Please rewrite the program so that the readout is aligned for all qubits, in a way that ensures that the idle time is the same for all qubits. This might involve some sort of "right-aligning" for the pi-pulses if they have different durations which I'm not sure is possible. If it's not, just assume the pi-pulses have the same duration.
If it still isn't clear, reference the refactored Ramsey node or this section of the confluence page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the proper changes. Let's discuss this.
@@ -187,56 +160,13 @@ class Parameters(NodeParameters): | |||
|
|||
# %% {Data_analysis} | |||
# Fit the exponential decay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you call a function called fit_exponential_decay
, you definitely don't need a comment which says
# Fit the exponential decay
. Please remove comments like this throughout the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks nicer to have "white space" like new lines for readability than having redundant comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -245,7 +175,6 @@ class Parameters(NodeParameters): | |||
and tau_error.sel(qubit=q.name).values / float(tau.sel(qubit=q.name).values) < 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic isn't so clear to me, and it's hard to read at first glance. I'm sure it's the same for you and any other users. Could you spend a minute or two trying to make sense of it, then split the if
-statement into something like this:
t1_is_positive = float(tau.sel(qubit=q.name).values) > 0
t1_XYZ = (tau_error.sel(qubit=q.name).values /
float(tau.sel(qubit=q.name).values)) < 1
if (t1_is_positive and t1_is_XYZ):
q.T1 = ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I think the goal is to check the error in the fit is smaller than the actual fit value. Probably done to verify that the fit make sense. I will ask Akiva.
settings["port"] = self.network["port"] | ||
self.qmm = QuantumMachinesManager(**settings) | ||
|
||
if self.network["cloud"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break everyone's code unless they have a field called cloud
in self.network
.
In order to avoid this, the safer option would be:
if self.network.get("cloud", False)
This treats "cloud" as an optional parameter of the dictionary, which resolves to False
if it's not present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
from quam_libs.lib.save_utils import fetch_results_as_xarray | ||
from quam_libs.lib.fit import fit_decay_exp, decay_exp | ||
|
||
def fetch_dataset(job: QmJob, qubits: List[Transmon], idle_times: np.ndarray, unit: unit) -> xr.Dataset: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's clearer to exlclude the unit
object as an argument to this function.
Either re-instantiate it in the function, or do the unit conversion manually since it is trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
No description provided.