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

Z2pack with elk #112

Draft
wants to merge 57 commits into
base: main
Choose a base branch
from
Draft

Conversation

euclidmenot2
Copy link

The kpoints.py file was modified to add an interface to Elk, an all-electron DFT code (http://elk.sourceforge.net/). Example Elk and Z2Pack input files for Bi2Se3 were also added.

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #112 (6b1bd5c) into dev/current (a87eee5) will decrease coverage by 1.39%.
The diff coverage is 14.81%.

❗ Current head 6b1bd5c differs from pull request most recent head 284b57c. Consider uploading reports for the commit 284b57c to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           dev/current     #112      +/-   ##
===============================================
- Coverage        93.36%   91.96%   -1.40%     
===============================================
  Files               37       37              
  Lines             1492     1519      +27     
===============================================
+ Hits              1393     1397       +4     
- Misses              99      122      +23     
Impacted Files Coverage Δ
z2pack/fp/kpoint.py 84.00% <14.81%> (-15.19%) ⬇️

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 a87eee5...284b57c. Read the comment docs.

@greschd
Copy link
Member

greschd commented Mar 25, 2021

Hi @euclidmenot2,

First off, thanks for taking the time to contribute!

It seems there are some problems with the pre-commit hooks. Instead of running them through continuous integration for every change, it is much quicker to run them locally. You can follow the following steps (some of which you may have already done):

  • clone the repository: git@github.com:euclidmenot2/Z2Pack.git
  • go into the Z2Pack directory: cd Z2Pack
  • check out the correct branch: git checkout z2packWithElk
  • create a virtual environment in which to install Z2Pack (instructions differ depending on which method you prefer, e.g. conda or virtualenv)
  • Install Z2Pack with development dependencies pip install -e .[dev]
  • Activate the pre-commit hooks: pre-commit install
  • Run the hooks: pre-commit run --all-files

The last command should automatically format the code, and run the other style checkers.

Once you're done:

  • Add the code: git add --all
  • Check which files were added with git status
  • Commit the code, and push: git commit; git push

Let me know if that works for you, or you're stuck somewhere. Unfortunately we don't have a comprehensive contributing guide that would cover these things.

@euclidmenot2
Copy link
Author

euclidmenot2 commented Mar 25, 2021 via email

@greschd
Copy link
Member

greschd commented Mar 25, 2021

Great! Yes, the codecov check tests for whether the changed code was covered by tests. In principle we can override it if needed, but it would be indeed good to have some tests.

Indeed, the Elk output files probably shouldn't be committed to the repository. Still, there are some ways in which we can add tests - following what is done for the other first-principles codes. There are two kinds of test:

  • The tests for the k-point function, located here: https://github.com/Z2PackDev/Z2Pack/blob/dev/current/tests/fp/test_fp_kpoint.py. These should be possible to add also with elk. When you add new tests there, it will create files with a "reference" the first time that is run. That can be inspected for correctness, and committed to the repository if it is ok.

  • Full tests, which run the first-principles code. Currently, the setup is such that they can only be run locally. This is a bit sub-optimal, but I haven't yet taken on the complexity of getting the codes themselves to run in CI. You can see an example at https://github.com/Z2PackDev/Z2Pack/blob/dev/current/tests/fp/test_espresso_new.py. There's even a hard-coded path to the code in there... something which I should fix. For now, you can do the same for Elk.

    There is a helper compare_wcc which checks that the WCC result does not change after the first time it has run. Again, the idea is to manually check that the first-principles "plugin" works, and then validate that it keeps working with the test.

    The tests are "marked"

    @pytest.mark.qe

    which means these tests will be skipped unless the --quantumespresso flag is passed to the test run. These markers are defined in conftest.py:

    Z2Pack/tests/conftest.py

    Lines 21 to 25 in a87eee5

    parser.addoption(
    '--quantumespresso',
    action='store_true',
    help='run Quantum ESPRESSO tests'
    )

    Z2Pack/tests/conftest.py

    Lines 39 to 41 in a87eee5

    config.addinivalue_line(
    "markers", "qe: mark tests which run with Quantum ESPRESSO"
    )

    The same kind of marker can be added for Elk.

The tests can be executed by running pytest from the root of the project, or with a specific test file as argument (e.g. pytest tests/fp/test_fp_kpoint.py).

Hope this gets you a step further, happy to help out if you're running into problems.
-Dominik

@euclidmenot2
Copy link
Author

euclidmenot2 commented Apr 5, 2021

Hi Dominik,

I am still having issues with Github and codecov on my Windows machine (I run my calculations on a separate Linux cluster that I do not run Github on). When I try to clone Z2pack using the Github Desktop app on my Windows machine, I get clone errors:
image

My workaround was to download as a *.zip, modify the files, run pylint and yapf, then re-upload. However, I haven't been able to get codecov to work with this workaround. Would you have any advice?

Thanks,

Lisa

Note: I now have the new Conda environment installed with the elk development version of Z2pack. However, I am not in a Git repository because of the aforementioned issues. It now won't let me activate and the pre-commit hooks. I could use some help when it comes to pushing the changes since the Github cloning seems to be having some issues.

Lisa

@euclidmenot2
Copy link
Author

To elaborate more, with my *.zip work-around, when I update test_fp_kpoint.py. and then re-upload it with an elk valid_lines (the VASP valid lines with a few name changes), it doesn't seem as though the reference data is being created (I am getting "Reference data does not exist value errors). I think that this is happening since I am using Continuous Integration and might be avoided if I could get Git to work nicely with Windows. Would you have any advice? I'm now trying to track down a Linux machine that is not our cluster.

Thanks again,
Lisa

@greschd
Copy link
Member

greschd commented Apr 6, 2021

Hi Lisa,

I think the reason you're getting the "Reference data does not exist" errors is that this "reference data" should be added to the .pytest_cache/v directory: https://github.com/Z2PackDev/Z2Pack/tree/dev/current/.pytest_cache/v

When you run the tests locally, these files are generated the first time you run them. Note that not the entire .pytest_cache directory should be added, only the test_<test_name> directories.

The reason why these files need to be added is that we make use of the "caching" feature in pytest to store reference data. The pytest-regressions package would be a better way of doing this, but I haven't gotten around to migrating this yet.

It seems the error you're getting in the git GUI is also somehow related to the .pytest_cache directory, although I can't quite figure out from the screenshot what is wrong.

Finally, if you need access to a Linux system on your Windows machine, you could try the "Windows Subsystem for Linux": https://docs.microsoft.com/en-us/windows/wsl/

Dominik

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