Update config access in calculate_channel_frequencies#742
Merged
Conversation
In Qiskit#695 we updated the usage of the backend configuration to avoid to_dict() and dict based access. All the access was supposed to be updated to be class attribute based instead. However that PR neglected to update the usage of the calculate_channel_frequencies method. This is causing an error when using the method now. This commit corrects the oversight and adds a unit test covering running that method when building a pulse system model from a backend to avoid this in the future. Fixes Qiskit#741
Contributor
|
@mtreinish thanks for these updates. There are actually a couple of other files that need updating - the I'll update these files and also add a test that would have caught this, then open a PR to your branch. |
…hannelLO object (instead of dict)
…ency calculations in resulting system_model to catch any formatting issues
Contributor
|
I've just implemented the changes from the previous comments and have opened a PR to this branch @mtreinish |
Additional fix dict updates Made the following changes: - Further updates to duffing_model_generators and corresponding tests to switch over to using UchannelLO instead of the dictionary version. - Added additional test in test_duffing_model_generators to call PulseSystemModel.calculate_channel_frequencies - this test will now catch any errors resulting from duffing_model_generators using an invalid format for specifying u channel los. - Fixed a bug found in writing the above test: in duffing_model_generators, if a coupling strength was 0.0 it would cause an error as it was ultimately being input as None when compiling the PulseSystemModel object, due to incorrect use of python's or function. - Modified a couple parts of @mtreinish 's PR to directly construct UchannelLO objects (as opposed to using the from_dict method.
DanPuzzuoli
approved these changes
May 12, 2020
Contributor
DanPuzzuoli
left a comment
DanPuzzuoli
left a comment
There was a problem hiding this comment.
I think this now covers all necessary changes to resolve this issue.
chriseclectic
approved these changes
May 14, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In #695 we updated the usage of the backend configuration to avoid
to_dict() and dict based access. All the access was supposed to be
updated to be class attribute based instead. However that PR neglected
to update the usage of the calculate_channel_frequencies method. This is
causing an error when using the method now. This commit corrects the
oversight and adds a unit test covering running that method when
building a pulse system model from a backend to avoid this in the
future.
Details and comments
Fixes #741
Co-authored-by: Daniel Puzzuoli dan.puzzuoli@gmail.com