-
Notifications
You must be signed in to change notification settings - Fork 83
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
Allow explicitly specifying 'ibrav'. #503
Allow explicitly specifying 'ibrav'. #503
Conversation
eace20f
to
f0d21b6
Compare
@giovannipizzi should the "cell to parameter" conversion go into One thing to consider here is that there are no checks in that function itself that the cell conforms to the specified |
f0d21b6
to
edf2532
Compare
@greschd yes, maybe as they are general they can be put into qe-tools? In this case I agree testing should be done by qe-tools, and raise there. |
AFAIK For inclusion in |
Indeed, we first want to release |
Ok, what's your opinion on the interface of this PR, specifically the way specifying If you're fine with this I will add tests and documentation. |
I don't have the time for it today, unfortunately, sorry. I am live demo'ing AiiDA in two hours and then have to prepare a proposal. Will try to look at it soon |
No worries, it's not an urgent thing. Good luck on the live demo! |
@greschd yeah sorry, you're right. |
edf2532
to
01f7f65
Compare
I guess we should first release qe_tools before we can finalise this, right? |
That's the plan 😄 |
7db57eb
to
f7a7d2f
Compare
@sphuber what's the best place to put the documentation for this feature? Maybe a subsection in https://aiida-quantumespresso.readthedocs.io/en/latest/user_guide/get_started/examples/pw_tutorial.html#structure could work, but I'm not sure if there might be a better place. |
To be honest, I haven't yet been able to look at the documentation and it badly needs to be improved I feel. So I don't have an opinion really. Just put it wherever you feel is best for now. |
f7a7d2f
to
7d87936
Compare
Ok, I've added a short paragraph in |
8cd83ac
to
79a525e
Compare
03a4d91
to
e4d4c42
Compare
e4d4c42
to
0043c3a
Compare
Rebased on the latest |
31897da
to
bda0881
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @greschd . Looking good. Just some minor suggestions and request to add a test.
1f56896
to
f3c07b5
Compare
If an 'ibrav' value other than zero is specified in the SYSTEM inputs, the cell of the input structure is converted into the appropriate A, B, C, cosAB, cosAC, cosBC values. As a check, the cell is reconstructed using qe_tools. The cells are compared element-wise, with an absolute tolerance controlled by the 'IBRAV_CELL_TOLERANCE' setting.
f3c07b5
to
08f2f4f
Compare
Fixes #347.
If an
ibrav
value other than zero is specified in theSYSTEM
inputs, the cell of the input structure is converted into the appropriate A, B, C, cosAB, cosAC, cosBC values. As a check, the cell is reconstructed using qe_tools. The cells are compared element-wise, with an absolute tolerance controlled by theIBRAV_CELL_TOLERANCE
setting.Still needs testing and documentation. No need to check the correctness yet, but I'm soliciting feedback on the interface.
Note also: This adds
numpy
andscipy
as explicit dependencies. They're implicit dependencies already, so I don't think that should be problematic.TODO:
add tests for the conversion between cell and parametersthese will go intoqe-tools
directlyibrav
featureqe_tools
?