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

Unitcell stress test: coordination should not change with full-rank supercells #117

Merged
merged 2 commits into from
Oct 16, 2020

Conversation

pablosanjose
Copy link
Owner

@pablosanjose pablosanjose commented Oct 15, 2020

This is the result of a stress test on the unitcell routines to catch edge cases. We want to guarantee that any unitcell(h, sc) with any full-rank sc does not change the coordination of the resulting Hamiltonian. To achieve that we needed to correct some bugs and edge cases. Long story short, we were assuming supercell definitions that were slightly different when building the superlattice and the Hamiltonian (position-based vs dn-based). Now both should be consistent (position-based). We also need to be strict about which sites end up in the final lattice: they should be only those within the bounds of the supercell, no hanging sites. Also, we needed to be a little more conservative in the scan of dn space when building the supercell.

Now tests over a full range of full-rank supercells generate the correct lattices, with identical coordination.

@pablosanjose pablosanjose changed the title Unitcell test Unitcell stress test: coordination should not change with full-rank supercells Oct 15, 2020
@pablosanjose
Copy link
Owner Author

pablosanjose commented Oct 15, 2020

The fast paths for unitcells (unitell(h, 3) and unitcell(h, 3,3,3)) was still using the dn-based criterion for supercell construction, that then conflicted with the hamiltonian construction (position-based). We remove the fastpaths for now. An advantage is that the definition of the supercell does not depend on whether we're using the fastpath: it's always the sites located within the supercell bounding box, drawn so that the site position average mean(r) is at the center of the supercell.

@pablosanjose
Copy link
Owner Author

The above strict definition of supercell has some unexpected consequences, such as unitcell(lat) != lat.unitcell. The reason is that lat.unitcell can have site positions outside the bounding box of the unitcell, as defined by the Bravais vectors. In this case, the outlying sites will be folded back onto the unitcell, yielding a different set of positions.

Thinking about all possible edge cases this seems unavoidable. If we allow sites outside the unitcell bounding box (as we should), one needs to be very disciplined about how supercells are built. Otherwise one can easily end up not finding the outliers in a dn search to build the final supercell (i.e. in the folding-back process). Hence, the current version of _cell_iter goes to great pains to (1) always cover a minimum dn bounding box that covers the supercell unit and (2) persist in the dn search if a site folds down to a dn that is not in the already searched domain.

remove obsolete tests

removed unitcell fastpaths for the moment

shortcut

remove more obsolete fastpath tests

test fix

more edge cases catch all outliers

remove test

fix test
@codecov-io
Copy link

codecov-io commented Oct 16, 2020

Codecov Report

Merging #117 into master will increase coverage by 0.11%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   62.00%   62.11%   +0.11%     
==========================================
  Files          16       16              
  Lines        2845     2856      +11     
==========================================
+ Hits         1764     1774      +10     
- Misses       1081     1082       +1     
Impacted Files Coverage Δ
src/Quantica.jl 100.00% <ø> (ø)
src/lattice.jl 81.81% <97.36%> (+0.22%) ⬆️
src/hamiltonian.jl 79.62% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8758326...01c6489. Read the comment docs.

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