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 VQC training with warm_start=True #312

Merged
merged 11 commits into from
Feb 12, 2022

Conversation

declanmillar
Copy link
Contributor

@declanmillar declanmillar commented Feb 10, 2022

Summary

Fixes the extraction of the initial_point in TrainableModel from the final point of the
minimization in the existing OptimizerResult object. Closes #296.

Adds unit tests to ensure VQC correctly handles warm starts.

Details and comments

As pointed out by @woodsp-ibm, "In Terra the optimizers were refactored and the old optimize method, that returned a tuple (subscriptable) was deprecated and a new method minimize was created that returns OptimizerResult object."

This is a very small fix to reflect that change and some related unit tests that would have caught the issue.

extract the initial_point in TrainableModel from the final point of the
minimization in the existing OptimizerResult object.
@declanmillar declanmillar changed the title Fix VQC warm_start=True Fix VQC training with warm_start=True Feb 10, 2022
@coveralls
Copy link

coveralls commented Feb 10, 2022

Pull Request Test Coverage Report for Build 1829125173

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 83.64%

Files with Coverage Reduction New Missed Lines %
qiskit_machine_learning/datasets/gaussian.py 3 35.0%
Totals Coverage Status
Change from base Build 1829048709: -0.06%
Covered Lines: 2817
Relevant Lines: 3368

💛 - Coveralls

Copy link
Member

@woodsp-ibm woodsp-ibm left a comment

Choose a reason for hiding this comment

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

Can you add a reno release note with a fixes section. This is required for issues that impact/affect the public API.

Thanks for adding a unit test that checks this. This is something that we always want if we find our CI has a hole like this case where the tests did not catch it. I see you have modelled it with data after what I see is done elsewhere here. I will note that that data values could be more directly coded into the data e.g directly have an instance of the optimizer etc rather than having this indirect via string values. ddt can also input to multiple parameters and do unpacking via decorator so the test does not need to have that. I am not asking for anything to be changed merely pointing this flexibility out that you may want to look at going forwards if/when using ddt. Eg from our former Aqua code https://github.com/Qiskit/qiskit-aqua/blob/c1564af8792c6664670807614a378147fd04d28f/test/aqua/test_vqe.py#L126-L131

@declanmillar
Copy link
Contributor Author

This PR should be labelled as Changelog: Bugfix and stable backport potential. I don't appear to have the necessary permissions to apply these myself.

@adekusar-drl adekusar-drl added 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 labels Feb 11, 2022
@adekusar-drl adekusar-drl merged commit 5a02c76 into qiskit-community:main Feb 12, 2022
mergify bot pushed a commit that referenced this pull request Feb 12, 2022
* fixes #296 warm_start with vqc

extract the initial_point in TrainableModel from the final point of the
minimization in the existing OptimizerResult object.

* update copyright

* add units tests to check vqc warm_start

* fix grammar

* fix grammar

* initialise -> initialize

* fix typo in test docstring

* add a reno for fix of #296

* fix quotes in reno report

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
(cherry picked from commit 5a02c76)
adekusar-drl pushed a commit that referenced this pull request Feb 12, 2022
* fixes #296 warm_start with vqc

extract the initial_point in TrainableModel from the final point of the
minimization in the existing OptimizerResult object.

* update copyright

* add units tests to check vqc warm_start

* fix grammar

* fix grammar

* initialise -> initialize

* fix typo in test docstring

* add a reno for fix of #296

* fix quotes in reno report

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
(cherry picked from commit 5a02c76)

Co-authored-by: Declan Millar <declan.millar@ibm.com>
@declanmillar declanmillar deleted the 296-vqc-warm-start branch February 15, 2022 12:01
oscar-wallis pushed a commit that referenced this pull request Feb 16, 2024
* fixes #296 warm_start with vqc

extract the initial_point in TrainableModel from the final point of the
minimization in the existing OptimizerResult object.

* update copyright

* add units tests to check vqc warm_start

* fix grammar

* fix grammar

* initialise -> initialize

* fix typo in test docstring

* add a reno for fix of #296

* fix quotes in reno report

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@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.

The warm_start=True attribute in the VQC class is broken
4 participants