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

Add probability normalization to DynamicsBackend sampling routine #239

Merged

Conversation

DanPuzzuoli
Copy link
Collaborator

Summary

Closes #237

Details and comments

When looking at the implementation I realized that two normalizations are necessary; it isn't possible to only normalize the probability before sampling. The state needs to be normalized before calculating probabilities so that the QuantumState.probabilities_dict function doesn't raise an error, and the resultant probabilities need to then be normalized to avoid numerical error in the probability computation.

As a result of this, I've kept options.normalize_states, and modified the code that samples the outcomes to also normalize the probabilities if options.normalize_states == True. I've modified the description of the normalize_states option to describe this, and added a "fixes" release note for this.

@to24toro
Copy link
Contributor

I am a little bit uncomfortable that normalize_state also computes the normalization of probability. Do you have any reason to continue using normalize_state as the variable name?

@DanPuzzuoli
Copy link
Collaborator Author

My thinking on the name is:

  1. Whatever we call this option, it makes sense for it to normalize at multiple points (the states before computing probabilities, and also the probabilities right before the sampling happens).
  2. The current option for specifying normalization is normalize_states, so it seemed simpler to just keep that name, rather than changing it to normalize_probabilities.

If you'd like I'm open to changing it to normalize_probabilities though; I do think it's a better name. If so I can make this change and also add an upgrade release note indicating that this change has occurred. I don't think with this I will bother following the deprecation procedure as I highly doubt anyone is currently using this option.

@to24toro
Copy link
Contributor

Thank you for your comment.
If few users will use and change this option to False, we can leave it as is for a while. We can consider about this naming issue again when demand arises to use normalize_state and normalize_probability separately.

@DanPuzzuoli DanPuzzuoli merged commit 5465b09 into qiskit-community:main Jul 12, 2023
DanPuzzuoli added a commit to DanPuzzuoli/qiskit-dynamics that referenced this pull request Aug 2, 2023
DanPuzzuoli added a commit that referenced this pull request Aug 8, 2023
* adding sympy as an explicit requirement (#233)

* Add warning for JAX versions on import of Dynamics (#232)

* Add probability normalization to DynamicsBackend sampling routine (#239)

* Add warning if digital carrier exceeds Nyquist frequency in pulse -> signal conversion (#242)

* Fix bug with carrier_freq being a JAX tracer if envelope is constant in Signal (#247)

* Remove subsystem_labels option from DynamicsBackend (#248)

* Rename subsystem_dims Dict to subsystem_dims_dict (#250)

* Fix ClassicalRegister counting in DynamicsBackend (#252)

* temporary fix for docs (#253)

* Drop support for backendV2 in from_backend (#249)

* incrementing version number

* cleaning up reno files

---------

Co-authored-by: Kento Ueda <[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.

The probability solved in Dynamicsbackend exceeds 1.
2 participants