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

Chemistry related bug fixes #1099

Merged
merged 6 commits into from
Jan 20, 2024
Merged

Chemistry related bug fixes #1099

merged 6 commits into from
Jan 20, 2024

Conversation

amccaskey
Copy link
Collaborator

@amccaskey amccaskey commented Jan 17, 2024 •
edited
Loading

UCCSD bug fixes, implementation adapted from @marwafar

Fixes #952

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 17, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@amccaskey
Copy link
Collaborator Author

amccaskey commented Jan 17, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@amccaskey
Copy link
Collaborator Author

amccaskey commented Jan 17, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@github-actions GitHub Actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jan 18, 2024
@cuda-quantum-bot
Docs preview for PR #1099.
79e924f
bmhowe23
bmhowe23 reviewed Jan 18, 2024
View reviewed changes
Copy link
Collaborator

@bmhowe23 bmhowe23 left a comment

Choose a reason for hiding this comment

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

Do our chemistry interfaces need to remain stable? I.e., should the old function interfaces (generateExcitations, singletExcitation, doubletExcitation, uccsd_num_parameters, and uccsd) be retained and/or deprecated? We normally try to do that for other interfaces, but I'm not sure about this one.

@amccaskey
Copy link
Collaborator Author

Do our chemistry interfaces need to remain stable? I.e., should the old function interfaces (generateExcitations, singletExcitation, doubletExcitation, uccsd_num_parameters, and uccsd) be retained and/or deprecated? We normally try to do that for other interfaces, but I'm not sure about this one.

No. These will ultimately be moved to library code that builds atop cuda-quantum outside of this repository. With this PR I'm simply fixing a bug so that users of the current code can still use it while the libraries are being implemented.

1tnguyen
1tnguyen approved these changes Jan 19, 2024
View reviewed changes
Copy link
Collaborator

@1tnguyen 1tnguyen left a comment

Choose a reason for hiding this comment

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

Looking good. Fixing a tricky bug in UCCSD 👍

@amccaskey
add extra test per review
086cffe 
Signed-off-by: Alex McCaskey <[email protected]>
@amccaskey
Copy link
Collaborator Author

amccaskey commented Jan 19, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@amccaskey amccaskey enabled auto-merge (squash) January 19, 2024 18:16
@github-actions GitHub Actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jan 19, 2024
@cuda-quantum-bot
Docs preview for PR #1099.
5373c23
@amccaskey
Copy link
Collaborator Author

amccaskey commented Jan 20, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@github-actions GitHub Actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jan 20, 2024
@cuda-quantum-bot
Docs preview for PR #1099.
c1bc4db
bmhowe23
bmhowe23 approved these changes Jan 20, 2024
View reviewed changes
Copy link
Collaborator

@bmhowe23 bmhowe23 left a comment

Choose a reason for hiding this comment

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

👍

@amccaskey amccaskey merged commit add7101 into NVIDIA:main Jan 20, 2024
133 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 20, 2024
@bmhowe23 bmhowe23 added the bug fix To be listed under Bug Fixes in the release notes label Jan 20, 2024
@bettinaheim bettinaheim added this to the release 0.6.0 milestone Jan 22, 2024
@bettinaheim bettinaheim changed the title UCCSD Bug Fixes #952 Chemistry related bug fixes Jan 23, 2024
@amccaskey amccaskey deleted the uccsdFix branch February 29, 2024 20:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marwafar marwafar marwafar left review comments

@1tnguyen 1tnguyen 1tnguyen approved these changes

@bmhowe23 bmhowe23 bmhowe23 approved these changes

@bettinaheim bettinaheim Awaiting requested review from bettinaheim bettinaheim is a code owner

@schweitzpgi schweitzpgi Awaiting requested review from schweitzpgi

@anthony-santana anthony-santana Awaiting requested review from anthony-santana

Assignees
No one assigned
Labels
bug fix To be listed under Bug Fixes in the release notes
Projects
None yet
Milestone
release 0.6.0
Development

Successfully merging this pull request may close these issues.

UCCSD code in the latest version (0.4.1) is incorrect
6 participants
@amccaskey @marwafar @1tnguyen @bmhowe23 @bettinaheim @anthony-santana