Skip to content

Remove get plot method #6

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 4, 2019
Merged

Remove get plot method #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
502ddc4
removed get_plot_segments public method
May 30, 2019
7f78a2a
fixed the modified plot data array method
May 30, 2019
749bc3c
loop replacedment tested with notebooks
May 31, 2019
18d3788
returned plot_dictionary from 2 if blocks
May 31, 2019
47cb297
lint friendly version to return plot_dictionary
May 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion qctrlopencontrols/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

from .dynamic_decoupling_sequences import (DynamicDecouplingSequence,
new_predefined_dds,
convert_dds_to_driven_controls)
convert_dds_to_driven_control)
from .driven_controls import DrivenControl, new_predefined_driven_control
from .qiskit import convert_dds_to_quantum_circuit
from .cirq import (convert_dds_to_cirq_circuit,
Expand Down
113 changes: 43 additions & 70 deletions qctrlopencontrols/driven_controls/driven_control.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,54 +31,6 @@
UPPER_BOUND_DURATION, LOWER_BOUND_DURATION)


def get_plot_data_from_segments(segments):
Copy link
Contributor

@charmasaur charmasaur May 31, 2019

Choose a reason for hiding this comment

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

By deleting this we're changing the API in a backwards-incompatible way, so could potentially break other packages that depend on open controls. I'm not sure how releases of the package are handled -- do we need a version bump or something?

Copy link
Contributor Author

@rajibchakravorty rajibchakravorty May 31, 2019

Choose a reason for hiding this comment

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

Actually this method was never used anywhere and hence should be removed anyway.

Copy link
Contributor

@charmasaur charmasaur May 31, 2019

Choose a reason for hiding this comment

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

Not used anywhere by us, but what if there's a third party package depending on it?

Copy link
Contributor Author

@rajibchakravorty rajibchakravorty May 31, 2019

Choose a reason for hiding this comment

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

This was a helper function to another method that actually gets the data for plotting. So the user should use this other method instead. I am pretty sure this should not be a problem.

Copy link
Contributor

@charmasaur charmasaur May 31, 2019

Choose a reason for hiding this comment

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

OK, sounds good. As this package matures I expect we'll need to introduce a proper versioning scheme, so that we can have proper deprecation policies for this sort of change, but in these early days I can't imagine too much going wrong.

"""
Generates arrays that can be used to produce a plot representing the shape of the driven control
constructed from the segments.

Parameters
----------
segments : list
List of segments formatted as described in qctrlopencontrols.driven_controls.DrivenControl

Returns
-------
tuple
Tuple made up of arrays for plotting formatted as
(amplitude_x,amplitude_y,amplitude_z,time) where:
- amplitude_k is the amplitude values.
- times the time corresponding to each amplitude_k coordinate.
Note that plot will have repeated times and for amplitudes, this is because it is
expected that these coordinates are to be used with plotting software that 'joins
the dots' with linear lines between each coordinate. The time array gives the x
values for all the amplitude arrays, which give the y values.

"""
segment_times = np.insert(np.cumsum(segments[:, 3]), 0, 0.)
coords = len(segment_times)
coord_amplitude_x = np.concatenate([[0.], segments[:, 0], [0.]])
coord_amplitude_y = np.concatenate([[0.], segments[:, 1], [0.]])
coord_amplitude_z = np.concatenate([[0.], segments[:, 2], [0.]])
plot_time = []
plot_amplitude_x = []
plot_amplitude_y = []
plot_amplitude_z = []
for i in range(coords):
plot_time.append(segment_times[i])
plot_time.append(segment_times[i])
plot_amplitude_x.append(coord_amplitude_x[i])
plot_amplitude_x.append(coord_amplitude_x[i + 1])
plot_amplitude_y.append(coord_amplitude_y[i])
plot_amplitude_y.append(coord_amplitude_y[i + 1])
plot_amplitude_z.append(coord_amplitude_z[i])
plot_amplitude_z.append(coord_amplitude_z[i + 1])

return (np.array(plot_amplitude_x),
np.array(plot_amplitude_y),
np.array(plot_amplitude_z),
np.array(plot_time))


class DrivenControl(QctrlObject): #pylint: disable=too-few-public-methods
"""
Creates a driven control. A driven is a set of segments made up of amplitude vectors
Expand Down Expand Up @@ -545,43 +497,64 @@ def get_plot_formatted_arrays(self, coordinates=CARTESIAN, dimensionless_rabi_ra
ArgumentsValueError
Raised when an argument is invalid.
"""
if coordinates not in [CARTESIAN, CYLINDRICAL]:
raise ArgumentsValueError(
'Unsupported coordinates provided: ',
arguments={'coordinates': coordinates})

if dimensionless_rabi_rate:
normalizer = self.maximum_rabi_rate
else:
normalizer = 1

if coordinates == CARTESIAN:
(x_amplitudes, y_amplitudes, detunings, times) = get_plot_data_from_segments(
np.vstack((self.amplitude_x / normalizer, self.amplitude_y / normalizer,
self.detunings, self.durations)).T
)
control_segments = np.vstack((
self.amplitude_x / normalizer,
self.amplitude_y / normalizer,
self.detunings,
self.durations)).T
elif coordinates == CYLINDRICAL:
control_segments = np.vstack((
self.rabi_rates / normalizer,
self.azimuthal_angles,
self.detunings,
self.durations)).T

segment_times = np.insert(np.cumsum(control_segments[:, 3]), 0, 0.)
plot_time = (segment_times[:, np.newaxis] * np.ones((1, 2))).flatten()
Copy link
Contributor

@charmasaur charmasaur May 31, 2019

Choose a reason for hiding this comment

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

optional: since this trick for duplicating the arrays is used in a few places, and isn't super intuitive (to me, anyway) you could pull it out into a little private helper function (which could even be defined inside this method). That lets you save some code, and implicitly gives a little bit of documentation (in the function name).

plot_amplitude_x = control_segments[:, 0]
plot_amplitude_y = control_segments[:, 1]
plot_amplitude_z = control_segments[:, 2]

plot_amplitude_x = np.concatenate(
([0.], (plot_amplitude_x[:, np.newaxis] * np.ones((1, 2))).flatten(), [0.]))
plot_amplitude_y = np.concatenate(
([0.], (plot_amplitude_y[:, np.newaxis] * np.ones((1, 2))).flatten(), [0.]))
plot_amplitude_z = np.concatenate(
([0.], (plot_amplitude_z[:, np.newaxis] * np.ones((1, 2))).flatten(), [0.]))

plot_dictionary = {}
if coordinates == CARTESIAN:
plot_dictionary = {
'amplitudes_x': x_amplitudes,
'amplitudes_y': y_amplitudes,
'detunings': detunings,
'times': times
}
'amplitudes_x': plot_amplitude_x,
'amplitudes_y': plot_amplitude_y,
'detunings': plot_amplitude_z,
'times': plot_time}

elif coordinates == CYLINDRICAL:
(x_plot, y_plot, detunings, times) = get_plot_data_from_segments(
np.vstack((self.rabi_rates / normalizer, self.azimuthal_angles,
self.detunings, self.durations)).T
)
if coordinates == CYLINDRICAL:

x_plot = plot_amplitude_x
y_plot = plot_amplitude_y
x_plot[np.equal(x_plot, -0.0)] = 0.
y_plot[np.equal(y_plot, -0.0)] = 0.
azimuthal_angles_plot = np.arctan2(y_plot, x_plot)
amplitudes_plot = np.sqrt(np.abs(x_plot**2 + y_plot**2))

plot_dictionary = {
'rabi_rates': amplitudes_plot,
'azimuthal_angles': azimuthal_angles_plot,
'detunings': detunings,
'times': times
}
else:
raise ArgumentsValueError(
'Unsupported coordinates provided: ',
arguments={'coordinates': coordinates})

'detunings': plot_amplitude_z,
'times': plot_time}
return plot_dictionary

def __str__(self):
Expand Down
2 changes: 1 addition & 1 deletion qctrlopencontrols/dynamic_decoupling_sequences/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@

from .dynamic_decoupling_sequence import DynamicDecouplingSequence
from .predefined import new_predefined_dds
from .driven_controls import convert_dds_to_driven_controls
from .driven_controls import convert_dds_to_driven_control
2 changes: 1 addition & 1 deletion qctrlopencontrols/dynamic_decoupling_sequences/driven_controls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def _check_maximum_rotation_rate(
'allowed_maximum_detuning_rate': UPPER_BOUND_DETUNING_RATE})


def convert_dds_to_driven_controls(
def convert_dds_to_driven_control(
Copy link
Contributor

@charmasaur charmasaur May 31, 2019

Choose a reason for hiding this comment

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

Same story here -- if an external package is using the old method, they'll now be broken (which might be fine, I'm just not sure).

Copy link
Contributor Author

@rajibchakravorty rajibchakravorty May 31, 2019

Choose a reason for hiding this comment

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

This is a valid concern. This method was public. IMHO - we should be more consistent in naming from the start.

dynamic_decoupling_sequence=None,
maximum_rabi_rate=2*np.pi,
maximum_detuning_rate=2*np.pi,
Expand Down
4 changes: 2 additions & 2 deletions qctrlopencontrols/dynamic_decoupling_sequences/dynamic_decoupling_sequence.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
QCTRL_EXPANDED, CSV, CYLINDRICAL)

from .constants import (UPPER_BOUND_OFFSETS, MATPLOTLIB)
from .driven_controls import convert_dds_to_driven_controls
from .driven_controls import convert_dds_to_driven_control


class DynamicDecouplingSequence(QctrlObject): #pylint: disable=too-few-public-methods
Expand Down Expand Up @@ -292,7 +292,7 @@ def export_to_file(self, filename=None,
integration with Q-CTRL BLACK OPAL's 1-Qubit workspace.
"""

driven_control = convert_dds_to_driven_controls(
driven_control = convert_dds_to_driven_control(
dynamic_decoupling_sequence=self,
maximum_rabi_rate=maximum_rabi_rate,
maximum_detuning_rate=maximum_detuning_rate,
Expand Down
24 changes: 12 additions & 12 deletions tests/test_dynamical_decoupling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import numpy as np
from qctrlopencontrols.exceptions import ArgumentsValueError
from qctrlopencontrols import (
DynamicDecouplingSequence, convert_dds_to_driven_controls)
DynamicDecouplingSequence, convert_dds_to_driven_control)


def _remove_file(filename):
Expand Down Expand Up @@ -298,10 +298,10 @@ def test_conversion_to_driven_controls():

_maximum_rabi_rate = 20*np.pi
_maximum_detuning_rate = 20*np.pi
driven_control = convert_dds_to_driven_controls(dd_sequence,
maximum_rabi_rate=_maximum_rabi_rate,
maximum_detuning_rate=_maximum_detuning_rate,
name=_name)
driven_control = convert_dds_to_driven_control(dd_sequence,
maximum_rabi_rate=_maximum_rabi_rate,
maximum_detuning_rate=_maximum_detuning_rate,
name=_name)

assert np.allclose(driven_control.rabi_rates, np.array(
[0., _maximum_rabi_rate, 0., 0., 0.,
Expand Down Expand Up @@ -336,7 +336,7 @@ def test_free_evolution_conversion():

_maximum_rabi_rate = 20 * np.pi
_maximum_detuning_rate = 20 * np.pi
driven_control = convert_dds_to_driven_controls(
driven_control = convert_dds_to_driven_control(
dd_sequence,
maximum_rabi_rate=_maximum_rabi_rate,
maximum_detuning_rate=_maximum_detuning_rate,
Expand Down Expand Up @@ -368,7 +368,7 @@ def test_free_evolution_conversion():

_maximum_rabi_rate = 20 * np.pi
_maximum_detuning_rate = 20 * np.pi
driven_control = convert_dds_to_driven_controls(
driven_control = convert_dds_to_driven_control(
dd_sequence,
maximum_rabi_rate=_maximum_rabi_rate,
maximum_detuning_rate=_maximum_detuning_rate,
Expand Down Expand Up @@ -405,10 +405,11 @@ def test_export_to_file():

_maximum_rabi_rate = 20 * np.pi
_maximum_detuning_rate = 20 * np.pi
driven_control = convert_dds_to_driven_controls(dd_sequence,
maximum_rabi_rate=_maximum_rabi_rate,
maximum_detuning_rate=_maximum_detuning_rate,
name=_name)
driven_control = convert_dds_to_driven_control(
dd_sequence,
maximum_rabi_rate=_maximum_rabi_rate,
maximum_detuning_rate=_maximum_detuning_rate,
name=_name)

_filename = 'dds_qctrl_cylindrical.csv'
driven_control.export_to_file(
Expand Down Expand Up @@ -448,6 +449,5 @@ def test_export_to_file():
_remove_file('dds_qctrl_cartesian.json')



if __name__ == '__main__':
pass