Remove get plot method #6
Conversation
|
Closing it now; The task is not complete. |
|
| UPPER_BOUND_DURATION, LOWER_BOUND_DURATION) | ||
|
|
||
|
|
||
| def get_plot_data_from_segments(segments): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Actually this method was never used anywhere and hence should be removed anyway.
There was a problem hiding this comment.
Not used anywhere by us, but what if there's a third party package depending on it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| def convert_dds_to_driven_controls( | ||
| def convert_dds_to_driven_control( |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
This is a valid concern. This method was public. IMHO - we should be more consistent in naming from the start.
| plot_amplitude_z = np.concatenate( | ||
| ([0.], (plot_amplitude_z[:, np.newaxis] * np.ones((1, 2))).flatten(), [0.])) | ||
|
|
||
| plot_dictionary = { |
There was a problem hiding this comment.
IMO it'd be a bit clearer to put this in an "if coordinates == CARTESIAN" block, so we're only setting plot_dictionary once (alternatively, we could just return the dictionary directly from the two if blocks, there's no need for the local variable at all).
There was a problem hiding this comment.
@charmasaur - I agree. Linter objects to some other ways. Just updated the method that may be a bit clearer and linter friendly.
| 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() |
There was a problem hiding this comment.
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).
charmasaur
left a comment
charmasaur
left a comment
There was a problem hiding this comment.
Looks good, up to you whether you want to do the array-doubling helper function.
| UPPER_BOUND_DURATION, LOWER_BOUND_DURATION) | ||
|
|
||
|
|
||
| def get_plot_data_from_segments(segments): |
There was a problem hiding this comment.
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.
# This is the 1st commit message: starting pyquil support # This is the commit message #1: pyquil support added # This is the commit message #2: first version of pyquil notebook added # This is the commit message #3: pyquil dependency added to setup # This is the commit message #4: trying gcc install for pyquil # This is the commit message #5: replacing gcc install with build-essential # This is the commit message #6: forcing yes to install anything # This is the commit message #7: ignoring blocks in pyquil notebook for tests # This is the commit message #8: linted; unnecessary import removed from notebook # This is the commit message #9: removed all references of circuits and replaced those with programs; pyquil removed from method names # This is the commit message #10: notebook rerun after changes # This is the commit message #11: pyquil program conversion removes unnecessary method
# This is the 1st commit message: starting pyquil support # This is the commit message #1: pyquil support added # This is the commit message #2: first version of pyquil notebook added # This is the commit message #3: pyquil dependency added to setup # This is the commit message #4: trying gcc install for pyquil # This is the commit message #5: replacing gcc install with build-essential # This is the commit message #6: forcing yes to install anything # This is the commit message #7: ignoring blocks in pyquil notebook for tests # This is the commit message #8: linted; unnecessary import removed from notebook # This is the commit message #9: removed all references of circuits and replaced those with programs; pyquil removed from method names # This is the commit message #10: notebook rerun after changes # This is the commit message #11: pyquil program conversion removes unnecessary method # This is the commit message #12: Public methods are bundled as __all__; changed ported over to notebooks; base object removed; repr method created # This is the commit message #13: additional error handling for new repr method # This is the commit message #14: repr method updated with class instance as input
Major modification - public method get_plot_data_from_segments removed and its functions are absorbed in the get_plot_formatted_arrays method
Minor modification - convert_dds_to_driven_controls->convert_dds_to_driven_control and changes related to this.