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

Fix for reading spinful CHG/CHGCAR #717

Merged
merged 6 commits into from
Mar 20, 2024
Merged

Conversation

tfrederiksen
Copy link
Contributor

I encountered a problem when reading spinful CHG and CHGCAR files. I'm unsure if the file formats could have changed, but as far as I can see, in VASP version 5.4.4 only the latter format contains "augmentation occupancies" blocks.

I also added a couple of new tests with a nitric oxide molecule in the cell.

  • Closes #x
  • Added tests for new/changed functions?
  • Ran isort . and black . [24.2.0] at top-level
  • Documentation for functionality in docs/
  • Changes documented in CHANGELOG.md

Copy link
Owner

@zerothi zerothi left a comment

Choose a reason for hiding this comment

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

Some minor points that isn't fully clear to me, and most likely this part of the code was never tested: https://app.codecov.io/gh/zerothi/sisl/blob/main/src%2Fsisl%2Fio%2Fvasp%2Fchg.py#L76 ;)
So it could be it should always be used!

@tfrederiksen
Copy link
Contributor Author

Some minor points that isn't fully clear to me, and most likely this part of the code was never tested: https://app.codecov.io/gh/zerothi/sisl/blob/main/src%2Fsisl%2Fio%2Fvasp%2Fchg.py#L76 ;) So it could be it should always be used!

OK I see.

Could you bump sisl-files so the coverage tests will run after a rebase?

@tfrederiksen
Copy link
Contributor Author

Could you bump sisl-files so the coverage tests will run after a rebase?

Can I actually do it? (would like to know)

@zerothi
Copy link
Owner

zerothi commented Mar 19, 2024

Could you bump sisl-files so the coverage tests will run after a rebase?

Can I actually do it? (would like to know)

you should do it in this branch :)

@tfrederiksen
Copy link
Contributor Author

you should do it in this branch :)

Can you guide me how to do it?

@zerothi
Copy link
Owner

zerothi commented Mar 19, 2024

something like this:

cd <root sisl folder>
git submodule init # if files doesn't already exist and is populated
cd files
git pull
cd ..
git add files

done

@zerothi
Copy link
Owner

zerothi commented Mar 19, 2024

The idea is that the submodule should be understood as a file, the content being the commit hash. :)

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.66%. Comparing base (7dfe0ea) to head (f3bb97c).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #717      +/-   ##
==========================================
- Coverage   86.67%   86.66%   -0.02%     
==========================================
  Files         399      399              
  Lines       50718    50773      +55     
==========================================
+ Hits        43960    44001      +41     
- Misses       6758     6772      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zerothi zerothi merged commit b00d5e6 into zerothi:main Mar 20, 2024
7 of 8 checks passed
@tfrederiksen tfrederiksen deleted the vasp-chg branch March 20, 2024 07:37
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