qiskit-community / qiskit-dynamics

Tools for building and solving models of quantum systems in Qiskit
https://qiskit-community.github.io/qiskit-dynamics/
Apache License 2.0
106 stars 60 forks source link

Add probability normalization to DynamicsBackend sampling routine #239

Closed DanPuzzuoli closed 1 year ago

DanPuzzuoli commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.