Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Correct the UCCSD.compute_excitation_lists() #1201

Merged
merged 4 commits into from
Aug 15, 2020

Conversation

tsura-crisaldo
Copy link
Contributor

Summary

Correct the UCCSD.compute_excitation_lists(), when indicate args: active_occupied, active_unoccupied, it now gives the correct excitation lists.

Details and comments

Details of the issue and the result after fixed the issue:
https://qiskit.slack.com/archives/CB6C24TPB/p1597341799066200?thread_ts=1597133029.202500&cid=CB6C24TPB
https://qiskit.slack.com/archives/CB6C24TPB/p1597341799066400?thread_ts=1597133029.202500&cid=CB6C24TPB

This is the file to test and compare the old and new version of UCCSD.compute_excitation_lists():
Test the Corrected UCCSD.compute_excitation_lists().zip

@CLAassistant
Copy link

CLAassistant commented Aug 14, 2020

CLA assistant check
All committers have signed the CLA.

@woodsp-ibm
Copy link
Member

Thanks for supplying the fix. Can you please sign the CLA - the email/id you used on the commit should match your github email - maybe that you did not set it up in git locally and its defaulting to some locally generated email that does not match.
When I had mentioned tests in Slack we have these as unit tests in the test folder here. These are then run to assure the code works as expected. Anyway I created one locally, in order to test what you did as well as other behavior of the excitation list computation, to ensure it works as expected since its clear the bug you found was not covered by unit testing. I'll add it to your fork once the CLA is sorted so it can all come as a single contribution. Sound ok?

@woodsp-ibm woodsp-ibm 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 Aug 14, 2020
@woodsp-ibm woodsp-ibm added this to the 0.8 milestone Aug 14, 2020
@tsura-crisaldo
Copy link
Contributor Author

Sounds good!
But there is a problem when signing the CLA.
Follow your reply, I used git config -l --local and git config -l --local --show-origin to check the user.email in my qiskit-aqua repository, both of them show the same email as the email I currently use in my account (the @users.noreply.github.com one).
I also followed the way https://docs.github.com/en/github/committing-changes-to-your-project/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user to check the message in the blue question mark, but there is no blue question mark:
image
I also clicked the let us recheck many times.
It still shows the same message CLA not signed yet.

@woodsp-ibm
Copy link
Member

Just to double check here is some info from github around account emails https://docs.github.com/en/enterprise/2.13/user/articles/setting-your-commit-email-address-in-git

If I look at the commit log locally, in my clone of your fork, the email of the entry looks like ....macbook-pro.gateway (I did not post the full thing here but hopefully its enough that you can see what is going on.

@tsura-crisaldo
Copy link
Contributor Author

Thank you so much for your help!
I've signed the CLA now.

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.

Thanks once again for finding and fixing the bug and making the contribution here, as well as persevering with the CLA to get it signed.

@woodsp-ibm woodsp-ibm merged commit 0ea40d9 into qiskit-community:master Aug 15, 2020
@woodsp-ibm woodsp-ibm changed the title correct the UCCSD.compute_excitation_lists() Correct the UCCSD.compute_excitation_lists() Aug 15, 2020
@@ -0,0 +1,6 @@
---
ixes:
Copy link
Contributor

@jlapeyre jlapeyre Aug 20, 2020

Choose a reason for hiding this comment

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

There is a typo here: ixes rather than fixes

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I'll take care of it

Cryoris pushed a commit to Cryoris/qiskit-aqua that referenced this pull request Sep 3, 2020
* correct the UCCSD.compute_excitation_lists

* Add unit test for excitations and active space

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: woodsp <woodsp@us.ibm.com>
pbark pushed a commit to pbark/qiskit-aqua that referenced this pull request Sep 16, 2020
* correct the UCCSD.compute_excitation_lists

* Add unit test for excitations and active space

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: woodsp <woodsp@us.ibm.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

5 participants