TBmodels icon indicating copy to clipboard operation
TBmodels copied to clipboard

Add parser for tb.dat

Open qiaojunfeng opened this issue 3 years ago • 5 comments

Some other people seem interested in this, so let's push this to upstream repo :-)

fix #33

qiaojunfeng avatar Aug 29 '22 17:08 qiaojunfeng

Codecov Report

Merging #115 (9fabe2a) into trunk (0a158de) will decrease coverage by 0.21%. The diff coverage is 93.15%.

@@            Coverage Diff             @@
##            trunk     #115      +/-   ##
==========================================
- Coverage   96.35%   96.14%   -0.21%     
==========================================
  Files          10       10              
  Lines        1069     1142      +73     
==========================================
+ Hits         1030     1098      +68     
- Misses         39       44       +5     
Impacted Files Coverage Δ
src/tbmodels/_tb_model.py 95.10% <93.15%> (-0.18%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Sep 02 '22 07:09 codecov[bot]

Sorry for the late reply, finally have some time to add a pytest.

Unfortunately, I cannot generate a tb.dat file that is fully consistent with the current hr.dat file in the pytest regression data, because

  1. the Hamiltonian depends on the gauge (from Wannierization)
  2. and most importantly, it seems the current W90 v3.1 generates a different set of R vectors (use_ws_distance = .true.) than the current wsvec.dat in regression data, see the new file here https://github.com/Z2PackDev/TBmodels/blob/130bf4f98d49d2c812b4f152fbaa0b815a17815e/tests/samples/silicontb_wsvec.dat#L139-L141 and the old file https://github.com/Z2PackDev/TBmodels/blob/130bf4f98d49d2c812b4f152fbaa0b815a17815e/tests/samples/silicon_wsvec.dat#L139-L142

So in the end I added new regression data for the tb.dat pytest, instead of comparing it with the existing regression data for hr dat.

Also, it seems the pre-commit has some version incompatibilities between black and click, and pylint. Not sure what are the versions you were using?

qiaojunfeng avatar Sep 15 '22 11:09 qiaojunfeng

If we generate a hr.dat, tb.dat and wsvec from the same Wannier90 run, can we add a test that compares the two method for loading the data? IMO we could also replace the previous regression data, since the load from hr.dat hasn't changed in this PR.

The pre-commit issue is due to https://github.com/psf/black/issues/2964; the easiest change is changing the click requirement to click>=7.0, !=7.1.0, <8.1 for the time being; I will update the pre-commit dependencies (and probably switch to poetry so we get consistent CI runs) in a separate PR. The pre-commit is fairly outdated though, and FMPOV it's fine if we just ignore it for this PR. I can upgrade and fix it in a separate PR.

greschd avatar Sep 15 '22 19:09 greschd

If we generate a hr.dat, tb.dat and wsvec from the same Wannier90 run, can we add a test that compares the two method for loading the data? IMO we could also replace the previous regression data, since the load from hr.dat hasn't changed in this PR.

Yes this is the best solution!

The pre-commit issue is due to psf/black#2964; the easiest change is changing the click requirement to click>=7.0, !=7.1.0, <8.1 for the time being; I will update the pre-commit dependencies (and probably switch to poetry so we get consistent CI runs) in a separate PR. The pre-commit is fairly outdated though, and FMPOV it's fine if we just ignore it for this PR. I can upgrade and fix it in a separate PR.

That would be great!

So, to proceed, maybe you could first update the pre-commit and poetry stuff, then I will make sure this PR doesn't break any of these checks. For the tb.dat file, either you regenerate the test data (tb.dat, and the hdf5 test files...), or I can regenerate and replace the old files, which one do you prefer?

qiaojunfeng avatar Sep 20 '22 09:09 qiaojunfeng

So, to proceed, maybe you could first update the pre-commit and poetry stuff

This is done, but produces a merge conflict because I switched to a src/ layout. Let me know if I should help out in rebasing / merging that.

can regenerate and replace the old files

If you could regenerate them with the latest Wannier90 would be great, thanks!

greschd avatar Sep 20 '22 21:09 greschd