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

Amplitude encoding should check if features are normalized #270

Closed
co9olguy opened this issue Jul 23, 2019 · 8 comments

Comments

@co9olguy
Copy link
Member

commented Jul 23, 2019

See this user report on pennylane discussion forum.

It appears that the default.qubit plugin code which performs approximate expectation estimation with finite shots can lead to probabilities which are invalid (greater than one). This leads to an exception when numpy.random.binomial is called. At the moment, it is unclear what is causing this, as the probability is generated via steps that should lead to a valid probability.

@johannesjmeyer

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

I'll have a look at how the sampling is implemented, maybe I'll spot something right away.

Also note that I changed the sampling behaviour in #256, maybe this will already solve this.

@johannesjmeyer johannesjmeyer self-assigned this Jul 23, 2019

@co9olguy

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

It seems from the user's feedback that this bug may be coming from something further upstream, but only manifests during this step because numpy checks for normalized probabilities. I don't know if merging in the changes to sampling will solve or just mask the original source of the issue 🤔

@johannesjmeyer

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

Maybe it is related to the following fact in AmplitudeEncoding:

The absolute square of all elements in ``features`` has to add up to one.

I can imagine that if that was not the case we could get problems downstream. If this is correct, then we should implement some sanity check in QubitStateVector which is directly underlying AmplitudeEncoding. Maybe also in both to have a more informative error message.

@johannesjmeyer johannesjmeyer added the bug label Jul 23, 2019

@co9olguy

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

Good find. I agree on all suggestions

@johannesjmeyer

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

I am actually somewhat dissatisfied with the function AmplitudeEncoding. Right now it's 98% percent superfluous because it has no advantage about using QubitStateVector directly (only that it has a speaking name).

We could probably beef up this function a bit -- by allowing a feature vector that has less than 2^n elements (we would then pad them with zeros) and allowing that the squares don't sum to 1 (we would then normalize the vector). What are your thoughts?

@josh146

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Rather than discuss features of AmplitudeEncoding unrelated to the bug here, best to make a new issue regarding it with a descriptive title.

@johannesjmeyer

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

@josh146, see #273

Okay, coming back to the error, maybe we should also have a sanity check in default.qubit that checks if the internal state is valid?

@AroosaIjaz

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Maybe it is related to the following fact in AmplitudeEncoding:

The absolute square of all elements infeatureshas to add up to one.

I can imagine that if that was not the case we could get problems downstream. If this is correct, then we should implement some sanity check in QubitStateVector which is directly underlying AmplitudeEncoding. Maybe also in both to have a more informative error message.

You are right. The test that checks if features used by AmplitudeEncoding is normalised was missing. I added one now in the PR #275 . I dont know if this fixes the error our user has, however.

@josh146 josh146 changed the title Default.qubit can give exceptions with shots > 1 due to poorly normalized probability Amplitude encoding should check if features are normalized Jul 26, 2019

@josh146 josh146 closed this Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.