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

[PBC Resources Estimates 1/4] Add k-point THC factorization #821

Merged
merged 21 commits into from
Aug 7, 2023

Conversation

fdmalone
Copy link
Collaborator

Splitting up #813 into separate PRs for easier review. See discussion there for high level picture. Here I am adding ISDF-THC factorization for the symmetry adapted two-electron integrals. A good entry point might be the notebook isdf.py.

@fdmalone fdmalone changed the title [PBC Resources Estimates 1/5] Add k-point THC factorization [PBC Resources Estimates 1/4] Add k-point THC factorization Jun 20, 2023
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

initial / preliminary comments.

Comment on lines 18 to 19
pytest.skip(f"Need pyscf for PBC resource estimates {err}",
allow_module_level=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if you try to import this a) without pyscf installed and b) not in the context of running tests with pytest? genuinely curious.

Is pyscf installed in the CI?

Copy link
Collaborator Author

@fdmalone fdmalone Jun 22, 2023

Choose a reason for hiding this comment

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

I did not know this but it throws an import/modulenotfound error (also the case for resource_estimates/init.py). This probably isn't very helpful. An alternative is to add pytest skipifs to the indivual unit tests conditional on the import, or I guess catch this throw from pytest.skip somehow and exit more gracefully. Seems like we need both.

pyscf is not, I think ideally we should conditionally run the tests for both this PBC code and the molecular resources (also not run through CI), where conditionally means if this code changes. The reason is some tests here are quite slow and can't really be made much faster AND the molecular code has some external dependencies which may require a more complicated build instruction. I looked into it and there did seem to be some actions tools out there to only run conditional on code in certain paths changing, but wasn't sure if there was a more sensible way to do this. Anyway I will open an issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

punting this to #825

@@ -0,0 +1,184 @@
# coverage: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this for the whole file? can we make it more specific? presumably some of these functions are tested in the tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eh actually, deleting this causes the workflow to fail, codecov complains the entire file is untested. Not sure why this is the case at the moment, given that the unit test tests the majority.

Comment on lines 19 to 20
"""Calculate the miller indices on a gamma centered non-1stBZ Monkorhst-Pack
mesh
Copy link
Collaborator

Choose a reason for hiding this comment

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

here and elsewhere in the file: really try to keep the first line of the docstring to be one line, followed by a blank line, and then the rest of the docstring. This affects docstring rendering in our and other doc tools

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry! There was some mishmash of docstring conventions going on. Lesson learned.

@@ -0,0 +1,20 @@
# coverage: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a better name than "utils" for all these modules. Everything should have some utility :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no utils!

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import itertools
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is gvec? add a short module description that explains this modules organization and grouping. A lot of the functions are for building maps? would it make sense to include map or map_builders as part of the module name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a little module docstring.

src/openfermion/resource_estimates/pbc/thc/utils/isdf.py Outdated Show resolved Hide resolved
src/openfermion/resource_estimates/pbc/thc/utils/isdf.py Outdated Show resolved Hide resolved
interp_indx: npt.NDArray) -> Tuple[npt.NDArray, npt.NDArray]:
"""Solve for interpolating vectors given interpolating points and orbitals.

Used for supercell and k-point so factor out as function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you telling me to "factor out as function"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Meant to say that this function is used by both which is why it's a standalone function.

src/openfermion/resource_estimates/pbc/thc/utils/isdf.py Outdated Show resolved Hide resolved
return zeta


def build_G_vectors(cell: gto.Cell) -> npt.NDArray:
Copy link
Collaborator

Choose a reason for hiding this comment

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

compare and contrast with gvec_logic.build_G_vectors. Are these supposed to have the same name?

Copy link
Collaborator Author

@fdmalone fdmalone Jul 22, 2023

Choose a reason for hiding this comment

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

They are essentially doing the same thing, but in gvec logic everything is in terms of the underlying integers that define the lattice vectors rather than the vectors here which include the primitive reciprocal lattice vectors directly. Think integer version vs floating point version.

max_iteration: int = 100,
threshold: float = 1e-6,
):
"""Initialize k-means solver to find interpolating points for ISDF.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's cvt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added doc.

@fdmalone
Copy link
Collaborator Author

@mpharrigan, I think I addressed them all PTAL.

@mpharrigan
Copy link
Collaborator

@fdmalone do you want to merge in the CI changes

@fdmalone
Copy link
Collaborator Author

fdmalone commented Aug 5, 2023

@mpharrigan Yes, I still need to merge and prune the slow tests.

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

cool! Thanks for tackling the CI work to get this (and existing code!) tested

@fdmalone fdmalone merged commit f1dfe9d into quantumlib:master Aug 7, 2023
11 checks passed
@fdmalone fdmalone deleted the pbc_kthc_factorization branch August 7, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants