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

Increase digits printed by plot_bvec #409

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

jaemolihm
Copy link
Contributor

@jaemolihm jaemolihm commented Mar 29, 2022

seedname.bvec file is used by EPW, and I found small but nonzero (~1E-6) relative error from the round-off errors in bvec. Such a small error rarely affects physics but it makes debugging tricky. So I believe it is worth changing.

In addition, testw90_bvec did not test the bvec data (it only checked the sizes). I updated the parser to check bvec data and updated the reference data.

@hjunlee
Copy link
Contributor

hjunlee commented Apr 3, 2022

@jaemolihm I already changed the number of digits here (#337) and for the moment, I don't want further changes.

In addition, this part is only related to EPW and I would like to ask all people who want to do MRs related to EPW in Wannier90 to directly contact me, the lead developer of EPW, before doing MRs.

@jaemolihm
Copy link
Contributor Author

jaemolihm commented Apr 4, 2022

@hjunlee This PR is a change to Wannier90 source code, not EPW source code. This code is in the Wannier90 upstream. Thus, I believe the code should be managed in the same way as other parts of Wannier90, i.e. as an openly-developed community code. I have followed the Wannier90 contributors guide and opened a Pull Requests.

(I think the situation is different from the case of EPW in QE, where the whole EPW/ folder is managed by the EPW team. For changes to the EPW source code, I will certainly follow your instruction.)

In addition, it is not correct that this part (subroutine plot_bvec) is related only to EPW. One can write the bvec file using the write_bvec flag, whether or not one is using EPW. I have faced problems using the seedname.bvec file for my own program (not EPW), thus this PR.

If the reason for objection is that such a change in printed digits is only a quick-and-dirty fix and one should aim for a more fundamental change (e.g. binary IO or directly accessing the variable via library mode), I would accept the argument and close this PR.

@jaemolihm
Copy link
Contributor Author

@hjunlee In the future, I will make sure I contact and notify you when I make a PR that would impact the EPW code.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Thanks @jaemolihm! This change is backward compatible and should not affect/break the behaviour of external codes, and I think it's an important improvement (and also comes with tests - thanks!), so I am going to merge this.
If this creates problems, we can discuss in a new issue.

@giovannipizzi giovannipizzi merged commit 1645e00 into wannier-developers:develop Feb 16, 2024
3 checks passed
@jaemolihm jaemolihm deleted the bvec_digits branch July 16, 2024 07:59
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.

3 participants