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

Add parser for tb.dat #115

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Conversation

qiaojunfeng
Copy link

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

fix #33

Copy link
Member

@greschd greschd left a comment

Choose a reason for hiding this comment

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

Thanks, @qiaojunfeng!

I haven't gotten around to properly reviewing this yet, so just one comment on the example.

Also, would you mind adding a unit test for this? Maybe it could produce the same result as one of the other parsing methods, so we can directly compare them?


if __name__ == "__main__":
WANNIER90_COMMAND = os.path.expanduser("~/git/wannier90/wannier90.x")
BUILD_DIR = "./build"
Copy link
Member

Choose a reason for hiding this comment

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

Here, I think we should either actually run the wannier90 executable, or provide the tb.dat file as an example.

@codecov
Copy link

codecov bot commented Sep 2, 2022

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%) ⬇️

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

@qiaojunfeng
Copy link
Author

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
    -3 1 1 6 1
    1
    4 0 -4

    and the old file
    -3 1 1 6 1
    2
    0 0 0
    4 0 -4

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?

@greschd
Copy link
Member

greschd commented Sep 15, 2022

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 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.

@qiaojunfeng
Copy link
Author

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?

@greschd
Copy link
Member

greschd commented Sep 20, 2022

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!

tbmodels/_tb_model.py Outdated Show resolved Hide resolved
tbmodels/_tb_model.py Outdated Show resolved Hide resolved
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.

Add method to parse from Wannier90 _tb.dat file
2 participants