[PBC Resources Estimates 1/4] Add k-point THC factorization#821
[PBC Resources Estimates 1/4] Add k-point THC factorization#821fdmalone merged 21 commits intoquantumlib:masterfrom
Conversation
fdmalone
changed the title
mpharrigan
left a comment
mpharrigan
left a comment
There was a problem hiding this comment.
initial / preliminary comments.
| pytest.skip(f"Need pyscf for PBC resource estimates {err}", | ||
| allow_module_level=True) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -0,0 +1,184 @@ | |||
| # coverage: ignore | |||
There was a problem hiding this comment.
is this for the whole file? can we make it more specific? presumably some of these functions are tested in the tests
There was a problem hiding this comment.
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.
| """Calculate the miller indices on a gamma centered non-1stBZ Monkorhst-Pack | ||
| mesh |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Sorry! There was some mishmash of docstring conventions going on. Lesson learned.
| @@ -0,0 +1,20 @@ | |||
| # coverage: ignore | |||
There was a problem hiding this comment.
is there a better name than "utils" for all these modules. Everything should have some utility :)
| # 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I've added a little module docstring.
| 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. |
There was a problem hiding this comment.
why are you telling me to "factor out as function"?
There was a problem hiding this comment.
Meant to say that this function is used by both which is why it's a standalone function.
| return zeta | ||
|
|
||
|
|
||
| def build_G_vectors(cell: gto.Cell) -> npt.NDArray: |
There was a problem hiding this comment.
compare and contrast with gvec_logic.build_G_vectors. Are these supposed to have the same name?
There was a problem hiding this comment.
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. |
|
@mpharrigan, I think I addressed them all PTAL. |
|
@fdmalone do you want to merge in the CI changes |
|
@mpharrigan Yes, I still need to merge and prune the slow tests. |
fdmalone
force-pushed
the
pbc_kthc_factorization
branch
from
7ee17fe to
6baeaf5
Compare
mpharrigan
left a comment
mpharrigan
left a comment
There was a problem hiding this comment.
cool! Thanks for tackling the CI work to get this (and existing code!) tested
2 participants
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.