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

Bug fix: Measurement properties automatic padding for DynamicsBackend initialization #209

Merged
merged 10 commits into from
Apr 28, 2023
Merged

Bug fix: Measurement properties automatic padding for DynamicsBackend initialization #209

merged 10 commits into from
Apr 28, 2023

Conversation

arthurostrauss
Copy link
Contributor

Summary

An indent was added in the reference code to avoid faulty behaviours when measurement_properties was added to the DynamicsBackend for the simulation to run properly. The bug was in the fact that the measurement schedule would be added, even though it had not been initialized properly (because the initialization was in a if statement with no else). Another check with a bool test was implemented to avoid loading an empty dictionary in case no measurement_properties was to be added.

Details and comments

Minor correction to an emerging bug

There was a indent mistake that could lead to an error when trying to add a measurement instruction with the pre-defined meas_schedule Schedule instance in the initialization of the DynamicsBackend object
@CLAassistant
Copy link

CLAassistant commented Mar 29, 2023

CLA assistant check
All committers have signed the CLA.

@DanPuzzuoli
Copy link
Collaborator

Thanks for this correction. There are two more things that should be done with this:

  • Add a test that verifies the functioning of this code block, that would have caught this bug. It probably makes the most sense to add it as a method to test.dynamics.backend.test_dynamics_backend.TestDynamicsBackend. E.g. have a check where no measurement instructions exist for any qubits, and another where some exist.
  • Create a release note file describing the bug fix. The process is described in the Release Notes section of CONTRIBUTING.md. You'll only need to fill in the bugfix section.

@DanPuzzuoli
Copy link
Collaborator

Another minor change that can be included in this PR is to move line 207 to before the for loop, as it is just redefining the same variable each time.

arthurostrauss and others added 2 commits April 16, 2023 22:05
It is now possible to automatically load a control_channel_map from an existing backend when declaring a DynamicsBackend with the from_backend() method. 

Tests of this features, as well as tests for the previous bug fix have been implemented.

For the new feature, note that manually setting the option control_channel_map of the backend will override the automatic loading of the control_channel_map (when DynamicsBackend is generated with from_backend() method)
@arthurostrauss
Copy link
Contributor Author

Hi, the requested information and tests, as well as the possibility to directly load the control_channels with the from_backend() method (tests are also implemented) have been added.
Let me know if additional changes are needed.

There is now an additional check that the provided backend actually has a control_channels attribute in its configuration. If it is not found, then no control_channel_map is added at the declaration (the user would then have to set it manually via the options).
Copy link
Collaborator

@DanPuzzuoli DanPuzzuoli left a comment

Choose a reason for hiding this comment

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

For now I'm just reviewing the control_channel_map construction code - once that's in order we can discuss the testing/documentation.

I've added a few in-line comments. The main thing is that this code is assuming the labels for control channels are two-tuples of qubit indices. We don't want this code to assume anything about the form of the labels, as control channels are, in principle, completely arbitrary and could be labelled by anything.

Aside from what's in the in-line comments, I have the following suggestion for tightening up the code:

control_channel_map_backend = None
if hasattr(backend, "control_channels"):
    control_channel_map_backend = backend.control_channels
elif hasattr(backend.configuration(), "control_channels"):
    control_channel_map_backend = backend.configuration().control_channels

if bool(control_channel_map_backend):
    # extract indices from channels
    control_channel_map_backend = {label: channel[0].index for label, channel in control_channel_map_backend.items()}

   # insert code constructing control_channel_map here

qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
Test was adapted accordingly to the changes.
It is now no longer possible to fill the control_channel_map from the coupling map of the backend. And the control_channels can carry more generic labels than the usual tuple (ctrl, tgt) qubits.
Copy link
Collaborator

@DanPuzzuoli DanPuzzuoli left a comment

Choose a reason for hiding this comment

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

Things are looking pretty good. I'm getting way more pedantic with my comments as I think it's good aside from minor polishing.

Aside from the comments I put directly in the code, the last ones are:

  • Use self.assertTrue(x) instead of assert x and self.assertEqual(x, y) instead of assert x == y, as this is the right way to do things in unittest.
  • Make sure test method doc strings are well formed sentences. There are some periods missing.

You will also need to run automatic formatting and linting, using terminal commands tox -e black and tox -e lint.

qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
qiskit_dynamics/backend/dynamics_backend.py Outdated Show resolved Hide resolved
test/dynamics/backend/test_dynamics_backend.py Outdated Show resolved Hide resolved
test/dynamics/backend/test_dynamics_backend.py Outdated Show resolved Hide resolved
test/dynamics/backend/test_dynamics_backend.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@DanPuzzuoli DanPuzzuoli left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@DanPuzzuoli DanPuzzuoli merged commit b673b7e into qiskit-community:main Apr 28, 2023
DanPuzzuoli added a commit to DanPuzzuoli/qiskit-dynamics that referenced this pull request Jun 8, 2023
DanPuzzuoli added a commit that referenced this pull request Jun 9, 2023
* Temporary upper bound on JAX version <=0.4.6 and update to DynamicsBackend tutorial (#210)

* Add links of API reference to tutorials and userguides (#212)

* Fix multiset ordering bug in perturbation module (#211)

Co-authored-by: Ian Hincks <[email protected]>

* Bump Sphinx Theme to 1.11 (#215)

* Bump Sphinx Theme to 1.11

* Also activate jquery

* Bug fix: Measurement properties automatic padding for DynamicsBackend initialization (#209)

Co-authored-by: Daniel Puzzuoli <[email protected]>

* bounding ipython version for compatibility with python 3.8 (#216)

* Update deploy_documentation.sh to deploy in ecosystem (#219)

* Bound Diffrax version (#226)

* Bounding diffrax and equinox versions. The latest versions require
the latest version of JAX, but due to an unresolved bug in JAX,
Dynamics is only compatible with jax<=0.4.6. This commit also
adds a release note stating exactly what versions of these packages
will work with the latest version of dynamics.

* Upgrade to qiskit_sphinx_theme 1.12 (#224)

* Update links to repo and documentation (#227)

* updating minor comments in pulse sim tutorial to remove confusion (#228)

* adding max step size argument to dynamics_backend tutorial, with explanation (#229)

* fixing typo

---------

Co-authored-by: Kento Ueda <[email protected]>
Co-authored-by: Ian Hincks <[email protected]>
Co-authored-by: Eric Arellano <[email protected]>
Co-authored-by: Arthur Strauss <[email protected]>
Co-authored-by: Luciano Bello <[email protected]>
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.

3 participants