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

Doci hamiltonian #704

Merged
merged 43 commits into from Feb 20, 2021
Merged

Doci hamiltonian #704

merged 43 commits into from Feb 20, 2021

Conversation

ncrubin
Copy link
Collaborator

@ncrubin ncrubin commented Jan 19, 2021

No description provided.

@google-cla google-cla bot added the cla: no label Jan 19, 2021
@ncrubin
Copy link
Collaborator Author

ncrubin commented Jan 19, 2021

@obriente I merged master and fixed some merge issues but it seems like this branch somehow has your very long commit history in it. Is it possible to compress like you did with the other PRs? It seems to have content from cvjjm about derivatives which might be from another project.

@google-cla
Copy link

google-cla bot commented Jan 20, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@obriente
Copy link
Collaborator

Hi Nick, taking a look through the commit history I think these are all relevant to this PR. (I think I've fixed the issues on my personal OF fork now.) Probably easiest to just leave this as is?

@ncrubin
Copy link
Collaborator Author

ncrubin commented Jan 27, 2021

@googlebot I fixed it.

@google-cla
Copy link

google-cla bot commented Jan 27, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@ncrubin
Copy link
Collaborator Author

ncrubin commented Jan 27, 2021

@cvjjm Can you consent?

@cvjjm
Copy link
Contributor

cvjjm commented Jan 27, 2021

I have the OK from Legal, but it was decided that we need to sign a company wide CLA and for that we need a caretaker and documentation.... The wheels are turning but it will take a few more days.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@cvjjm
Copy link
Contributor

cvjjm commented Feb 4, 2021

Changes look good to me. Unfortunately this PR is now also stuck because of the CLA. I already requested a CLA via the link https://cla.developers.google.com/ last Friday, but we haven't received anything to sign since. Have you had this happen previously? Unfortunately there is no contact on that page where I could ask what the status is.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ncrubin
Copy link
Collaborator Author

ncrubin commented Feb 18, 2021

@cvjjm It looks like you can now consent. Covestro is showing up in the CLA list.

@cvjjm
Copy link
Contributor

cvjjm commented Feb 19, 2021

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Feb 19, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: yes and removed cla: no labels Feb 19, 2021
@cvjjm
Copy link
Contributor

cvjjm commented Feb 19, 2021

Looks like we are finally ready to go!

@google-cla
Copy link

google-cla bot commented Feb 19, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@ncrubin
Copy link
Collaborator Author

ncrubin commented Feb 19, 2021

@cvjjm Oh no! tests failed when I updated the branch. This is also happening on one of my PRs where I changed 1 line. I will get to the bottom of this. Something else is broken...not this PR.

@ncrubin
Copy link
Collaborator Author

ncrubin commented Feb 19, 2021

In fact I think it is related to your _issmall PR. Was that passing tests?

@ncrubin
Copy link
Collaborator Author

ncrubin commented Feb 20, 2021

@cvjjm okay it is not your PR. The "bug" was introduced by the the 1.6.1 release of scipy (which was released 2 days ago). There is a change that fixes a dtype propagation in initializing sparse matrices. The tests that are failing all use sparse_tools.py methods that are initializing a sparse matrix with csc_matrix. Scipy's PR# 13403 fixes a bug with sparse matrix initialization. When tuples are passed to csc_matrix (which is a subclass of _cs_matrix) they eventually call coo_matrix for initialization. coo_matrix WAS using np.array(data) to cast the data for the coo_matrix. np.array is very clever sometimes. If you provide data as a list of floats and np.complex128s it is smart enough to cast the resulting array as the higher dtype--np.complex128. This is bad because it was ignoring the dtype that was passed to the coo_matrix constructor. Thus you are at the whim of however smart np.array is at determining dtypes. For us this was a blessing. We were getting defaulted into the right 'np.complex128' without knowing it. We weren't specifying the dtype so it defaulted to float and then at some point was changed to np.complex128 when np.array's cleverness kicked in. When the new scipy release got cut this bug was fixed and all of a sudden our tests that tried to initialize complex sparse matrices started throwing errors about casting to floats. We were trying to pass data with complex numbers to coo_matrix but without specifying the dtype. Thus scipy caught our bug of not correctly specifying the datatype.

More info can be found here (scipy/scipy#13403).

I will open a PR to fix the sparse routines and then circle back and merge this PR.

@google-cla
Copy link

google-cla bot commented Feb 20, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Feb 20, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@ncrubin ncrubin merged commit 40f4dd2 into master Feb 20, 2021
@ncrubin ncrubin deleted the doci_hamiltonian branch February 20, 2021 04:45
ncrubin added a commit to ncrubin/OpenFermion that referenced this pull request Jul 25, 2022
* adding a class to represent DOCI Hamiltonians

* Added new structure to DOCIHamiltonian class - tests currently not passing

* Wrote and passed tests for hr1+hr2+hc

* Few bugfixes

* Tests passing, wooo!

* Added a few tests, everything passes!

* Added addition and subtraction to DOCIHamiltonian

* Formatting

* Formatting

* Formatting

* Formatting

* Formatting

* Fixed typo

* Made n_body_tensors a property in PolynomialTensor (so that this can be overwritten)

* Shifted hr1, hr2, and hc matrices back into main class instead of individual classes; this is unnecessary with the slight changes to PolynomialTensor

* Started fixing bugs and writing tests

* Fixed bugs

* Formatting

* Rewrote X,Y,Z terms

* Changed get_tensor -> get_tensors

* Removed requirement that n_body_tensors is always set in PT class

* Fixed formatting, linting

* Fixed typo

* Formatting

* Improving coverage

* Format

* Adding new PT functionality and tests

* More coverage

* Updates following Christians comments

* Fixed linting

* Removed leftover debugging statement

Co-authored-by: Christian Gogolin <christian.gogolin@covestro.com>
Co-authored-by: Tom O'Brien <teobrien@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants