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.

Sign up for GitHub

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

Jump to bottom

fixes #297: fitting in batches without all labels #317

Merged
merged 11 commits into from
Feb 17, 2022
Merged

fixes #297: fitting in batches without all labels #317

merged 11 commits into from
Feb 17, 2022

Conversation

declanmillar
Copy link
Contributor

@declanmillar declanmillar commented Feb 14, 2022 •
edited
Loading

Summary

Closes #297. Currently, VQC will throw an error if trained on batches of data where not all of the target labels that can be found in the full dataset are present. This is because VQC interprets the number of unique labels in the current batch as the number of classes.

Currently, VQC is hard-coded to expect one-hot encoded labels. Therefore, I suggest a small change to determine the number of classes from the shape of the label array. I have also edited the docstring to add the label encoding requirements of the current VQC implementation.

Details and comments

Related issues that I will raise separately:

@coveralls
Copy link

coveralls commented Feb 14, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1855742845

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.689%
Totals Coverage Status
Change from base Build 1855602603: 0.0%
Covered Lines: 2827
Relevant Lines: 3378
💛 - Coveralls

@woodsp-ibm
Copy link
Member

Could we get a test case that would fail previously that will now work with this bug fix. Since its an aspect that CI did not catch it would be good to get a unit test in there to cover it.

Also don't forget the reno release note.

@woodsp-ibm woodsp-ibm added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Feb 14, 2022
@declanmillar declanmillar reopened this Feb 14, 2022
@declanmillar
Copy link
Contributor Author

Could we get a test case that would fail previously that will now work with this bug fix. Since its an aspect that CI did not catch it would be good to get a unit test in there to cover it.

Also don't forget the reno release note.

I've added a new unit test that would fail without this fix. It may be necessary to revise it if and when we implement a wider range of valid label encodings in the VQC interface. I've also added a reno note for the fix.

adekusar-drl
adekusar-drl previously approved these changes Feb 15, 2022
View reviewed changes
Copy link
Collaborator

@adekusar-drl adekusar-drl left a comment

Choose a reason for hiding this comment

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

Looks good to me!
@woodsp-ibm Do you have open question?

@declanmillar declanmillar mentioned this pull request Feb 15, 2022
Closed
dlasecki
dlasecki suggested changes Feb 15, 2022
View reviewed changes
test/algorithms/classifiers/test_vqc.py Outdated
Comment on lines 312 to 313
else:
quantum_instance = self.qasm_quantum_instance
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to make this if explicit in case someone adds other backends to the test input in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, seems sensible. Done.

@adekusar-drl
Copy link
Collaborator

@woodsp-ibm Is there anything left on your radar? If not, I'll approve and merge this PR.

woodsp-ibm
woodsp-ibm approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@woodsp-ibm woodsp-ibm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is there anything left on your radar? If not, I'll approve and merge this PR.

All looks good to me. May want to create issues for the other improvements/changes that are noted in the main comment so as we don't lose them.

@adekusar-drl adekusar-drl merged commit dcbd78c into qiskit-community:main Feb 17, 2022
@adekusar-drl
Copy link
Collaborator

@declanmillar could you please post an issue(s) mentioned in this PR?

mergify bot pushed a commit that referenced this pull request Feb 17, 2022
@declanmillar
fixes #297: fitting in batches without all labels (#317)
9dc4dc8 
* fixes #297: fitting in batches without all labels

* test vqc with incomplete batch labels

* uncomment other vqc tests

* add reno note for fix of issue #297

* fix typo

* remove docstring reference from reno note

* make quantum instance selection explicit for vqc tests

* assert that all batches assume the correct number of classes

* fix style

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
(cherry picked from commit dcbd78c)
@mergify mergify bot mentioned this pull request Feb 17, 2022
Merged
mergify bot added a commit that referenced this pull request Feb 17, 2022
@mergify @declanmillar
fixes #297: fitting in batches without all labels (#317) (#321)
00c1d77 
* fixes #297: fitting in batches without all labels

* test vqc with incomplete batch labels

* uncomment other vqc tests

* add reno note for fix of issue #297

* fix typo

* remove docstring reference from reno note

* make quantum instance selection explicit for vqc tests

* assert that all batches assume the correct number of classes

* fix style

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
(cherry picked from commit dcbd78c)

Co-authored-by: Declan Millar <declan.millar@ibm.com>
@declanmillar declanmillar deleted the 297-fix-batch-num-classes branch February 17, 2022 22:46
oscar-wallis pushed a commit that referenced this pull request Feb 16, 2024
@declanmillar @manoelmarques
fixes #297: fitting in batches without all labels (#317)
e254987 
* fixes #297: fitting in batches without all labels

* test vqc with incomplete batch labels

* uncomment other vqc tests

* add reno note for fix of issue #297

* fix typo

* remove docstring reference from reno note

* make quantum instance selection explicit for vqc tests

* assert that all batches assume the correct number of classes

* fix style

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adekusar-drl adekusar-drl adekusar-drl left review comments

@dlasecki dlasecki dlasecki requested changes

@woodsp-ibm woodsp-ibm woodsp-ibm approved these changes

@stefan-woerner stefan-woerner Awaiting requested review from stefan-woerner

@pbark pbark Awaiting requested review from pbark

@manoelmarques manoelmarques Awaiting requested review from manoelmarques

Assignees
No one assigned
Labels
stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Training the VQC in batches, where one batch happens to have only one label, leads to error
6 participants
@declanmillar @coveralls @woodsp-ibm @adekusar-drl @dlasecki @manoelmarques