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

Fix GroverOptimizer's rotation_count #132

Merged
merged 2 commits into from May 15, 2021
Merged

Fix GroverOptimizer's rotation_count #132

merged 2 commits into from May 15, 2021

Conversation

t-imamichi
Copy link
Collaborator

@t-imamichi t-imamichi commented May 14, 2021

Summary

Fixes #129

GroverOptimizer's rotation_count is set in a wrong way.
It is originally rotation_count = int(np.ceil(algorithm_globals.random.uniform(0, m - 1))), but it is always 1 if m=1.
It should be either 0 or 1. So, I replace it with algorithm_globals.random.integers(0, m).

The order of result.get_counts(qc) has an issue. It is sorted by probability, but some bitstrings may have the same probabilities and it results in a probabilistic behavior of unit tests on different environment (mac and linux).
This PR sorts the counts by bitstring order if qasm_simulator is used. So, the resulting iterator has the same order in any environment.
If statevector_simulator is used, it omits the sort because the results are generated in a sorted order.

Details and comments

@t-imamichi t-imamichi requested a review from a-matsuo May 14, 2021 03:09
@t-imamichi t-imamichi changed the title [WIP] Fix GroverOptimization's m Fix GroverOptimization's rotation_count May 14, 2021
@t-imamichi t-imamichi marked this pull request as ready for review May 14, 2021 03:28
@t-imamichi t-imamichi added the stable backport potential The bug might be minimal and/or import enough to be port to stable label May 14, 2021
@a-matsuo
Copy link
Contributor

LGTM! The code rotation_count = int(np.ceil(algorithm_globals.random.uniform(0, m - 1))) was not working as expected. We needed random integers including 0. However the code did not return 0 since it has ceil.

@t-imamichi t-imamichi changed the title Fix GroverOptimization's rotation_count Fix GroverOptimizer's rotation_count May 14, 2021
@t-imamichi
Copy link
Collaborator Author

A question is still left. A function is_good_state in grover_optimizer is used for making Grover object. But, I found that is_good_state is never used. Even if I put assert False in it, the unit tests pass. This is the reason why is_good_state has a wrong signature, which I fixed in this PR, but it did not raise an error in the past.

As a conclusion, we need overhaul GroverOptimizer.

* Simpify GroverOptimizer._measure

* Simplify unit tests of GroverOptimizer
@woodsp-ibm woodsp-ibm added the Changelog: Bugfix Include in the Fixed section of the changelog label May 14, 2021
@woodsp-ibm
Copy link
Member

woodsp-ibm commented May 14, 2021

A question is still left. A function is_good_state in grover_optimizer is used for making Grover object. But, I found that is_good_state is never used. Even if I put assert False in it, the unit tests pass. This is the reason why is_good_state has a wrong signature, which I fixed in this PR, but it did not raise an error in the past.

Looking at the coverage output from CI here, is_good_state code is still not covered by unit tests - it looks like its still not called at all despite being passed to Grover.

@t-imamichi
Copy link
Collaborator Author

I see. We should add a unit test of is_good_state in the future.

@woodsp-ibm
Copy link
Member

I see. We should add a unit test of is_good_state in the future.

I would have imagined that is_good_state should have been called by Grover during its normal operation. As such I am unsure why coverage shows that its not called - and your comment above indicated its not called since even with parameters mismatching on the signature it should get called, but may go wrong/exception. Does this need further investigation?

@t-imamichi
Copy link
Collaborator Author

Yes, we need further investigation about is_good_state in a separate issue. Since it does not seem to affect the existing unit tests, I think we can merge this PR.

@t-imamichi t-imamichi merged commit b0d411a into main May 15, 2021
@t-imamichi t-imamichi deleted the fix-grover-m2 branch May 15, 2021 02:09
mergify bot pushed a commit that referenced this pull request May 15, 2021
* Fix rotation_count in GroverOptimizer

* Simpify GroverOptimizer._measure

* Simplify unit tests of GroverOptimizer

(cherry picked from commit b0d411a)

# Conflicts:
#	qiskit_optimization/algorithms/grover_optimizer.py
#	test/algorithms/test_grover_optimizer.py
t-imamichi added a commit that referenced this pull request May 16, 2021
* cherry-pick 4bce259

* cherry-pick b0d411a

* fix style

Co-authored-by: a-matsuo <47442626+a-matsuo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the Fixed section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants